-
Couldn't load subscription status.
- Fork 1.5k
Type-check require declarations
#6286
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: trunk
Are you sure you want to change the base?
Conversation
| // CHECK:STDERR: ^~~~~~~ | ||
| interface Z(T:! type) { | ||
| // Self can not appear in a requirement. It can appear in the self-type or in | ||
| // an interface parameter. |
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.
Is this a rule? I don't recall this one. Do you know why this is disallowed?
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 is #6285, which needs some discussion. There is a TODO in the code where the diagnostic is generated.
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 have removed this restriction and diagnostic.
| // CHECK:STDERR: require T impls Z(Self); | ||
| // CHECK:STDERR: ^~~~~~~ | ||
| // CHECK:STDERR: | ||
| require T impls Z(Self); |
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 guess eventually the error we want here is that Z is not identified (if I remember correctly). Maybe worth a TODO?
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.
Josh says IIUC we want to be able to require itself, and it should just mean it uses all the require statements that were written above. When there's none then that doesn't work though.
| context.decl_introducer_state_stack().Push<Lex::TokenKind::Require>(); | ||
|
|
||
| auto scope_id = context.scope_stack().PeekNameScopeId(); | ||
| auto scope_inst_id = context.name_scopes().Get(scope_id).inst_id(); |
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.
Is this inst id always valid (eg, in a scope nested within a function scope)?
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 believe so, yes.
In this:
interface Y {}
fn F() {
interface Z {
require impls Y;
}
}
The scope inst is the Z decl.
If you meant:
fn F() {
require impls Y;
}
Then that's a parse error.
toolchain/check/handle_require.cpp
Outdated
| } | ||
| for (auto self_impls : facet_type_info.self_impls_constraints) { | ||
| type_iter.Add(self_impls); | ||
| } |
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 we be checking all of these interfaces mention Self, not just that any of them do? Eg require i32 impls X(Self) & Y should probably be invalid.
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.
Done, thanks
They don't get stored anywhere yet, but this type checks the declarations and diagnoses errors in their form, such as not placing a facet type after
implsor a type before it.