Add AST validation pass and move some checks to it #33794
Conversation
| pub impl Tr for S { //~ ERROR invalid visibility qualifier | ||
| pub fn f() {} //~ ERROR invalid visibility qualifier | ||
| pub const C: u8 = 0; //~ ERROR invalid visibility qualifier | ||
| pub type T = u8; //~ ERROR invalid visibility qualifier |
There was a problem hiding this comment.
This change seems to be for the worse, since pub is a valid qualifier, just unneccessary here
There was a problem hiding this comment.
The "unnecessary -> invalid" change is rolled back.
|
What is the value in having this as a separate pass, rather than being part of the AST->HIR lowering? One benefit seems to be separation of concerns, however, I can imagine wanting to assert a lot of the things checked by this pass in the lowering in any case, so that may be moot. Also, this should always be a small pass, since anything big will probably be better performed on the HIR, so it doesn't seem to bad to smoosh it together with HIR lowering. |
Pretty much this. Mixing lowering code (almost always successfull) with AST validation code (intended solely for error reporting) looks kind of messy. It's not a strong opinion though.
I wouldn't expect that, later compiler passes are mostly ready to deal with anything they can face in non-validated AST/HIR, so the lowering step can be faultless. I expect the larger part of errors generated by the validation pass to be non-fatal (for example the restriction on Hm, maybe I should rename |
|
r=me with sanity =-> validation |
petrochenkov
changed the title
|
Updated. |
|
@bors: r+ |
|
📌 Commit 91d8a4f has been approved by |
Add AST validation pass and move some checks to it The purpose of this pass is to catch constructions that fit into AST data structures, but not permitted by the language. As an example, `impl`s don't have visibilities, but for convenience and uniformity with other items they are represented with a structure `Item` which has `Visibility` field. This pass is intended to run after expansion of macros and syntax extensions (and before lowering to HIR), so it can catch erroneous constructions that were generated by them. This pass allows to remove ad hoc semantic checks from the parser, which can be overruled by syntax extensions and occasionally macros. The checks can be put here if they are simple, local, don't require results of any complex analysis like name resolution or type checking and maybe don't logically fall into other passes. I expect most of errors generated by this pass to be non-fatal and allowing the compilation to proceed. I intend to move some more checks to this pass later and maybe extend it with new checks, like, for example, identifier validity. Given that syntax extensions are going to be stabilized in the measurable future, it's important that they would not be able to subvert usual language rules. In this patch I've added two new checks - a check for labels named `'static` and a check for lifetimes and labels named `'_`. The first one gives a hard error, the second one - a future compatibility warning. Fixes #33059 ([breaking-change]) cc rust-lang/rfcs#1177 r? @nrc
|
💔 Test failed - auto-linux-64-opt-rustbuild |
|
Updated. |
|
☔ The latest upstream changes (presumably #33864) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Rebased. |
Add test for `::super` in import prefix
|
ping @nrc |
|
@bors: r=nrc |
|
📌 Commit 731144b has been approved by |
Add AST validation pass and move some checks to it The purpose of this pass is to catch constructions that fit into AST data structures, but not permitted by the language. As an example, `impl`s don't have visibilities, but for convenience and uniformity with other items they are represented with a structure `Item` which has `Visibility` field. This pass is intended to run after expansion of macros and syntax extensions (and before lowering to HIR), so it can catch erroneous constructions that were generated by them. This pass allows to remove ad hoc semantic checks from the parser, which can be overruled by syntax extensions and occasionally macros. The checks can be put here if they are simple, local, don't require results of any complex analysis like name resolution or type checking and maybe don't logically fall into other passes. I expect most of errors generated by this pass to be non-fatal and allowing the compilation to proceed. I intend to move some more checks to this pass later and maybe extend it with new checks, like, for example, identifier validity. Given that syntax extensions are going to be stabilized in the measurable future, it's important that they would not be able to subvert usual language rules. In this patch I've added two new checks - a check for labels named `'static` and a check for lifetimes and labels named `'_`. The first one gives a hard error, the second one - a future compatibility warning. Fixes #33059 ([breaking-change]) cc rust-lang/rfcs#1177 r? @nrc
The purpose of this pass is to catch constructions that fit into AST data structures, but not permitted by the language. As an example,
impls don't have visibilities, but for convenience and uniformity with other items they are represented with a structureItemwhich hasVisibilityfield.This pass is intended to run after expansion of macros and syntax extensions (and before lowering to HIR), so it can catch erroneous constructions that were generated by them. This pass allows to remove ad hoc semantic checks from the parser, which can be overruled by syntax extensions and occasionally macros.
The checks can be put here if they are simple, local, don't require results of any complex analysis like name resolution or type checking and maybe don't logically fall into other passes. I expect most of errors generated by this pass to be non-fatal and allowing the compilation to proceed.
I intend to move some more checks to this pass later and maybe extend it with new checks, like, for example, identifier validity. Given that syntax extensions are going to be stabilized in the measurable future, it's important that they would not be able to subvert usual language rules.
In this patch I've added two new checks - a check for labels named
'staticand a check for lifetimes and labels named'_. The first one gives a hard error, the second one - a future compatibility warning.Fixes #33059 ([breaking-change])
cc rust-lang/rfcs#1177
r? @nrc