-
Notifications
You must be signed in to change notification settings - Fork 309
[SP024] Implement some
keyword
#7229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[SP024] Implement some
keyword
#7229
Conversation
…aces missing the keyword
Implement sp024 lang 2026
…support Format code for PR shader-slang#7172
…ement-SP024-dyn-support Regenerate command line reference for PR shader-slang#7172
… yet. re-organize the validation as per review, did not fix cmd-line-option yet, that is next.
…support Format code for PR shader-slang#7172
Implement sp024 dyn support
…ire AST This is not a full implementation, still requires: 1. clean up ==> SomeTypeDecl is used in an unclean way in many places 2. coerce types 3. proper and complete tests 4. return type checks 5. complete `dyn Type`, `SomeType`, and `UnboundSomeType` var assignment
🌈 Formatted, please merge the changes from this PR |
…-support Format code for PR shader-slang#7229
source/slang/slang-ast-decl.h
Outdated
@@ -137,6 +137,21 @@ class LetDecl : public VarDecl | |||
FIDDLE(...) | |||
}; | |||
|
|||
// Represents the type `SomeType<IFoo>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update your comment to use the actual syntax.
e.g. represent some IFoo var;
but it's used as in/inout parameter of a function, e.g.
void func(in some IFoo var);
there the type must be a concrete type when the function is called.
UnboundSomeTypeDecl
represents some IFoo var;
but it's only used as out
function parameter, e.g.
void func(out some IFoo var);
so var
is unbounded some type, and it can be assigned by another some
type variable.
The reason we use bound and unbound is to distinguished those two cases, as only unbounded some type variable can be assigned value.
// Manages coerce rules for `SomeTypeDecl` and `UnboundSomeTypeDecl`. | ||
// Primarily this function diagnoses incorrect `SomeTypeDecl` coercing. | ||
// To coerce this function unwraps the inner interface type of a `SomeTypeDecl`. | ||
if (tryCoerceSomeType(site, toType, outToExpr, fromType, fromExpr, sink, outCost)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the convention, can you make the if condition surround this function
if (isDeclRefTypeOf<SomeTypeDecl>(toType) || isDeclRefTypeOf<SomeTypeDecl>(fromType))
{
return tryCoerceSomeType(...);
}
// If we have a SomeType on RHS, we are only allowed to copy if LHS is `dyn` | ||
if (auto someTypeDeclRef = isDeclRefTypeOf<SomeTypeDecl>(fromType)) | ||
{ | ||
if (isDynGivenType(toType)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if toType
is not dyn, will it have diagnostics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will error.
More specifically:
implicit_conversion
will error. This is because implicit conversion requires exactly the same type when resolving overloads throughcoerce
(overloadContext.disallowNestedConversions
), which is not possible withsome
type since all instances ofsome
are different.- Explicit conversion works fine.
source/slang/slang-check-decl.cpp
Outdated
@@ -34,6 +34,12 @@ static bool isAssociatedTypeDecl(Decl* decl) | |||
return false; | |||
} | |||
|
|||
static bool isSlang2025OrOlder(SemanticsVisitor* visitor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this equal to !isSlang2026OrLater()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is equivalent, I will change the semantics.
source/slang/slang-check-decl.cpp
Outdated
|
||
static void maybeCreateUnboundSomeTypeDeclFromVarDecl(VarDeclBase* varDecl, ASTBuilder* astBuilder) | ||
{ | ||
// If type is `out` or uninitialized we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so it doesn't matter whether it's initialized or not.
So you should update your comments.
just say if type is out
parameter or local variable, the some
type variable will be unbounded.
} | ||
} | ||
|
||
// not allowed `some` for return-type and paramDecl-type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is some
type not allowed for return and param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make it more clear, but this section of function is exclusively for dyn interface
types.
This is a restriction since if a dyn
type can return a some
type we will have to deal with dynamic dispatch and some
types (which goes against the goal of this feature).
if (allowExperimentalDynamicDispatch(visitor, optionSet)) | ||
return; | ||
|
||
if (!funcDecl->parentDecl->hasModifier<DynModifier>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand why checking parent decl of function decl, instead of funcDecl itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the function validates use of the funcDecl
as a child of a dyn interface
type.
source/slang/slang-check-decl.cpp
Outdated
{ | ||
if (auto someType = as<SomeTypeDecl>(decl)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think function is the container, as some
can only appear in a function.
@@ -2955,6 +2955,11 @@ Expr* SemanticsVisitor::checkGenericAppWithCheckedArgs(GenericAppExpr* genericAp | |||
{ | |||
return CreateErrorExpr(genericAppExpr); | |||
} | |||
if (as<SomeTypeExpr>(argExpr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is also disallowed, some IFoo var[2];
@@ -5577,6 +5576,12 @@ struct ExprLoweringVisitorBase : public ExprVisitor<Derived, LoweredValInfo> | |||
UNREACHABLE_RETURN(LoweredValInfo()); | |||
} | |||
|
|||
LoweredValInfo visitSomeTypeExpr(SomeTypeExpr* /*expr*/) | |||
{ | |||
SLANG_UNIMPLEMENTED_X("'some' type expression during code generation"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type expression during code generation"
-> type expression during IR lowering"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the suggestion, although I noticed every other function follows a different pattern (the pattern I used)
for example:
LoweredValInfo visitFuncTypeExpr(FuncTypeExpr* /*expr*/)
{
SLANG_UNIMPLEMENTED_X("type expression during code generation");
UNREACHABLE_RETURN(LoweredValInfo());
}
LoweredValInfo visitTupleTypeExpr(TupleTypeExpr* /*expr*/)
{
SLANG_UNIMPLEMENTED_X("type expression during code generation");
UNREACHABLE_RETURN(LoweredValInfo());
}
LoweredValInfo visitPointerTypeExpr(PointerTypeExpr* /*expr*/)
{
SLANG_UNIMPLEMENTED_X("'*' type expression during code generation");
UNREACHABLE_RETURN(LoweredValInfo());
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd maintain consistency with what is already in there, and maybe file an issue for somebody to go and clean up the terminology there to be more consistent. Honestly, though, this is the kind of thing that shouldn't ever surface for users of Slang, so I don't know if it's a high priority to fix.
/format |
🌈 Formatted, please merge the changes from this PR |
…-support Format code for PR shader-slang#7229
It is not a breaking change if it is only effective in 2026. 2026 is an experimental language version that we are actively exploring. No user code is expected to use 2026 right now. |
I don't quite understand "The given code does not infer generics". What's wrong/not working/special about the example code you gave? |
I reworded the PR statement for clarity. The code provided does not correctly have generics inferred in TOT, due to this, there are no tests on this functionality in this PR either. I wanted to explain "why we don't have tests for XYZ, but we have tests for ABC". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are big parts of how this PR approaches the implementation that I'd really like to see changed. It's possible that this approach could work in the short term with all of the many restrictions that are being placed on where some
is being allowed, but nearly all of that code is going to need to be scrapped as we scale up the feature, and in the interim I'm worried about there being a lot of very opaque logic here that doesn't seem like it will be easy to maintain.
These are difficult and subtle features that are getting added, and it is important that the implementation of such things should feel as close to "obviously correct" as is possible. Putting so much of the logic related to a feature for types on compiler code related to variables worries me a lot.
@@ -405,6 +405,10 @@ void DeclRefBase::toText(StringBuilder& out) | |||
getTargetType(getCurrentASTBuilder(), getParent())->toText(out); | |||
} | |||
} | |||
else if (auto someTypeDecl = as<SomeTypeDecl>(decl)) | |||
{ | |||
out << "some " << someTypeDecl->interfaceType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should a reference to a SomeTypeDelcl
for IFoo
just print out as some IFoo
(as seems to be implemented here), or should we find some way to distinguish the various distinct SomeTypeDecl
s that get created?
The concern would be that a user might get an error message like "cannot convert value of type some IFoo
to type some IFoo
" and be confused.
// Represents the type of `some Type` for `some Type varName;`. | ||
// This decl of `some Type` is not assignable since it already | ||
// has a given value. | ||
// The wrapped type is a concrete interface named `interfaceType`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little inaccurate...
The SomeTypeDecl
represents a particular (but unknown) concrete type, and is a declaration that is synthesized to represent the type indicated by a specific some ...
type expression. Each textually distinct some ...
type expression will result in the creation of a distinct SomeTypeDecl
.
The type represented by the SomeTypeDecl
is a concrete and proper type, and it is known to conform to the given interfaceType
(which must be an interface
type).
/// A type expression that represents a some type, e.g. `some T` | ||
FIDDLE() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might benefit from some expansion. E.g., saying that each textually distinct SomeTypeExpr
represents a kind of placeholder, standing in for an unknown concrete type (which will later be represented as a SomeTypeDecl
.
The SomeTypeExpr
is a type expression, but importantly it does not have a one-to-one correspondence with a subclass of Type
.
@@ -1204,6 +1204,16 @@ bool SemanticsVisitor::_coerce( | |||
return true; | |||
} | |||
|
|||
// Manages coerce rules for `SomeTypeDecl` and `UnboundSomeTypeDecl`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see a declaration for UnboundSomeTypeDecl
... maybe I just missed it?
static bool isDynType(Type* type) | ||
{ | ||
return !isDeclRefTypeOf<SomeTypeDecl>(type) && isDeclRefTypeOf<InterfaceDecl>(type); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this simplify to just isDeclRefTypeOf<InterfaceDecl>(type)
? That is, there should never be a type that is both a decl-ref to an InterfaceDecl
and to a SomeTypeDecl
, so it is fine to only check for the InterfaceDecl
here... I think...
/// Treat type as a `some` variable | ||
INST(SomeTypeDecoration, someTypeDecoration, 0, 0) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this IR encoding choice talked through with @csyonghe ? If it was, then I'll trust his judgement on this being the Right Way to approach it.
Otherwise... I'm a little concerned about yet again encoding the relevant information on variables instead of at the level of the types themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at later code has me even more confused, because the comment on this instruction says it causes a type to be treated as a "some
variable," and that led to me assuming it applied to variables and not types... but it turns out the decoration gets placed on types... so now I wonder what it means to treat a type as a "some
variable."
@@ -5577,6 +5576,12 @@ struct ExprLoweringVisitorBase : public ExprVisitor<Derived, LoweredValInfo> | |||
UNREACHABLE_RETURN(LoweredValInfo()); | |||
} | |||
|
|||
LoweredValInfo visitSomeTypeExpr(SomeTypeExpr* /*expr*/) | |||
{ | |||
SLANG_UNIMPLEMENTED_X("'some' type expression during code generation"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd maintain consistency with what is already in there, and maybe file an issue for somebody to go and clean up the terminology there to be more consistent. Honestly, though, this is the kind of thing that shouldn't ever surface for users of Slang, so I don't know if it's a high priority to fix.
if (!loweredType->findDecoration<IRSomeTypeDecoration>()) | ||
context->irBuilder->addDecoration(loweredType, kIROp_SomeTypeDecoration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like its just plain wrong and could easily cause a lot of problems.
The issue here is that the result of lowerType
on the underlying interface type is in most cases going to be an IRInterfaceType
, and in that case this logic is slapping teh IRSomeTypeDecoration
on every interface that ever gets used to form a some
type anywhere in the code.
I'm not sure what purpose that decoration could serve, then, because it would be impossible to tell apart the use sites that intend to use that interface in a some
-style fashion from those that want to use it in a dyn
-style fashion.
else if (AdvanceIf(parser, "some")) | ||
{ | ||
typeSpec.expr = ParseSomeExpr(parser); | ||
return typeSpec; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to at least try to register some
as a syntax declaration.
Look at all the uses of _makeParseExpr
further down in the same file, to see examples of how we add other keywords that introduce expression syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the benefit of anybody/everybody who is reading this and wondering why I care about the distinction:
The Slang compiler has a lot of infrastructure that allows it to take things that would be "keywords" or "reserved words" in other languages/compilers and just treat them as named declarations that, as much as possible, follow the same scoping rules as the rest of the language.
There are reasonable arguments to be made for and against the approach Slang is currently taking, but some of the reasons for why we are doing this are:
-
We can have/introduce keywords for compatibility with HLSL and GLSL without accidentally breaking a lot of user code. E.g.,
triangle
is a keyword in HLSL, and if we didn't allow user declarations to shadow/redeclare keywords, we'd be stopping folks from ever giving a variable that name. -
We can easily swap syntax in/out while keeping the same parser code. We don't currently make as much use of this as we could, but we can easily allow for different dialects of Slang to have different sets of keywords without changing the core parser code at all. It's even possible, in principle, to have user code influence what keywords it "sees" based on what built-in modules it
import
s (though we haven't implemented that kind of support) -
In theory we have a lot of the infrastructure we need in order to support user-defined syntax extensions that are treated no differently than built-in syntax (macros in the style of, e.g., Racket).
The long/short is that we are currently invested enough in this approach that the ideal would be for new syntax to be added via these mechanisms rather than via ad-hoc testing for a specific string in the lookahead while parsing.
auto declRefType = declRef.getDecl()->getType(); | ||
// Unwrap-type for substituting | ||
if (auto someTypeDeclRef = isDeclRefTypeOf<SomeTypeDecl>(declRefType)) | ||
{ | ||
auto subType = declRef.substitute( | ||
astBuilder, | ||
as<DeclRefType>(someTypeDeclRef.getDecl()->interfaceType)); | ||
SomeTypeDecl* newDecl; | ||
if (auto unboundSomeTypeDeclRef = isDeclRefTypeOf<UnboundSomeTypeDecl>(declRefType)) | ||
newDecl = astBuilder->create<UnboundSomeTypeDecl>(); | ||
else | ||
newDecl = astBuilder->create<SomeTypeDecl>(); | ||
newDecl->loc = declRef.getDecl()->loc; | ||
newDecl->interfaceType = TypeExp(subType); | ||
return DeclRefType::create(astBuilder, newDecl); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever this code is doing, I'm almost 100% sure it isn't right.
First, there is no circumstance in which querying the type of a variable should ever be creating new declarations via the ASTBuilder
. I'm not even sure how this is supposed to be correct in the first place, since distinct SomeTypeDecl
s represent distinct types, but with this logic every time somebody queries the type of a decl-ref to a variable they will get a distinct type.
I would like to put my foot down and say that all of this added code should be deleted, and then the rest of the PR should be changed so that the result is valid. The most important thing that seemingly has to be done is to give the SomeTypeDecl
s that get created proper parent declarations, so that decl-refs to them will properly include context related to their ancestor declarations (e.g., any outer generics).
ADDITIONALLY:
|
Fixes: #7144
Changes:
some
keywordSomeType
before opening existentials 2. unwrapSomeType
before applying substitutionsdyn
keyword validation (primarilyvarDecl
related)Note about missing feature, generics inference:
Note about file-tree for tests added: