-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Forbid unsized structs which are non-instantiable #16985
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
Conversation
E0160 | ||
E0160, | ||
E0162, | ||
E0163 |
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 did you skip E0161
?
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.
Because I added it in a different PR and it will make rebasing easier :-)
Addressed kballard's comments |
|
||
if !check_ty_may_be_sized(ccx.tcx, ty::node_id_to_type(ccx.tcx, it.id)) { | ||
span_err!(ccx.tcx.sess, it.span, E0163, | ||
"struct cannot be instantiated since it can never have statically known size"); |
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 still think the word "a" is necessary here. It should be more obvious if you get rid of the words "statically known": "it can never have size". Structs don't "have size", they "have a size". Therefore they should also "have a statically known size".
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 English in error messages is abbreviated, so it's slightly odd grammar. To my ears, omitting the 'a' sounds better. Even with it, the English is not great (it should be something like 'The struct Foo cannot be instantiated because its size can never be statically known by the compiler'),
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.
+1 to the 'a' here, even though it's chopped. I have a hard time parsing
this as written.
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.
Abbreviated grammar is fine. But the lack of an "a" looks like a straight-up error, not an abbreviation. It would be like emitting an error for let x;
with the string "this variable must have value", instead of the much more correct "this variable must have a value".=
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.
OK, I'll stick an 'a' in. 'size' does feel different to 'value' to me, but I can't explain why.
Now with more 'a's |
Turns out I can fix this ICE without banning always-unsized structs. So, closing for now. |
update: add editor/extension information to bug report template When attempting to reproduce issues, I encounter difficulties due to differences in versions of LSP clients and editors (such as rust-lang#16985, rust-lang#16867, and more) This sometimes consumes a lot of efforts from contributors to communicate the details about LSP client information. Therefore, I believe adding editor/extension information to the issue template would be helpful for problem reproduction.
update: add editor/extension information to bug report template When attempting to reproduce issues, I encounter difficulties due to differences in versions of LSP clients and editors (such as rust-lang#16985, rust-lang#16867, and more) This sometimes consumes a lot of efforts from contributors to communicate the details about LSP client information. Therefore, I believe adding editor/extension information to the issue template would be helpful for problem reproduction.
Closes #16977
r?