parse: merge fn syntax + cleanup item parsing - #68728
Conversation
This comment has been minimized.
This comment has been minimized.
commented
Feb 1, 2020
|
Other comments: Please avoid " Functions taking multiple
I don't see
The Looks like we are very close to using a single routine like trait Tr {
static S: u8 = 0;
}which currently look pretty bad in tests. |
commented
Feb 1, 2020
|
Meta: this PR also was too big to be review-able carefully. |
This comment has been minimized.
This comment has been minimized.
a170865 to
704ab0c
Compare
|
I made sure that all commits passed UI tests so it shouldn't be hard to take chunks and move them into separate PRs (with comments addressed). I'll try to do that in way that seems sensible. |
commented
Feb 2, 2020
I would prefer avoiding this as these enums are mostly unrelated flags. Merging them would result in making it harder to track what state is passed in and whatnot. Part of the reason for this refactoring was so that possibly getting rid of some of the flags would be facilitated and bitflags would make it harder than
The front matter is only the "effect qualifiers" (intrinsic to functions themselves, and unrelated to other item forms) + the
Didn't seem like they were used, but we can also recover insertion points from the start / ends as the order is fixed. |
This comment has been minimized.
This comment has been minimized.
eeee74c to
f8b5708
Compare
This comment has been minimized.
This comment has been minimized.
08b4e90 to
6879ce9
Compare
6879ce9 to
ad72c3a
Compare
commented
Feb 13, 2020
|
All remaining review comments have been addressed. |
commented
Feb 13, 2020
|
@bors r+ |
commented
Feb 13, 2020
|
📌 Commit ad72c3a has been approved by |
Here we continue the work in #67131 in particular to merge the grammars of
fnitems in various positions.A list of language level changes (as sanctioned by the language team in #65041 (comment) and #67131):
selfparameters are now syntactically allowed as the first parameter irrespective of item context (and in function pointers). Instead, semantic validation (ast_validation) is used.Syntactically,
fnitems inextern { ... }blocks can now have bodies (fn foo() { ... }as opposed tofn foo();). As above, we use semantic restrictions instead.Syntactically,
fnitems in free contexts (directly in a file or a module) can now be without bodies (fn foo();as opposed tofn foo() { ... }. As above, we use semantic restrictions instead, including for non-ident parameter patterns.const extern fnfeature gating is now done post-expansion such that we do not have conditional compatibilities of function qualifiers in parsing.The
FnFrontMattergrammar becomes:That is, all item contexts now syntactically allow
const async unsafe extern "C" fnand use semantic restrictions to rule out combinations previously prevented syntactically. The semantic restrictions include in particular:fns inextern { ... }can have no qualifiers.constandasynccannot be combined.To fuse the list-of-items parsing in the 4 contexts that items are allowed, we now must permit inner attributes (
#![attr]) insidetrait Foo { ... }definitions. That is, we now allow e.g.trait Foo { #![attr] }. This was probably an oversight due to not using a uniform parsing mechanism, which we now do have (fn parse_item_list). The semantic support (including e.g. for linting) falls out directly from the attributes infrastructure. To ensure this, we include a test for lints.Put together, these grammar changes allow us to substantially reduce the complexity of item parsing and its grammar. There are however some other non-language improvements that allow the compression to take place.
A list of compiler-internal changes (in particular noting the parser-external data-structure changes):
We use
enum AllowPlus/RecoverQPath/AllowCVariadic { Yes, No }inparser/ty.rsinstead of passing around 3 differentbools. I felt this was necessary as it was becoming mentally taxing to track which-is-which.fn visit_trait_itemandfn visit_impl_itemare merged intofn visit_assoc_itemwhich now is passed anAssocCtxtto check which one it is.We change
FnKindto:with:
This is then taken advantage of in tweaking the various semantic restrictions as well as in pretty printing.
In
ItemKind::Fn, we changeP<Block>toOption<P<Block>>.In
ForeignItemKind::Fn, we changeP<FnDecl>toFnSigandP<Block>toOption<P<Block>>.We change
ast::{Unsafety, Spanned<Constness>}>intoenum ast::{Unsafe, Const} { Yes(Span), No }respectively. This change in formulation allow us to excludeSpanin the case ofNo, which facilitates parsing. Moreover, we also add aSpantoIsAsyncwhich is renamed toAsync. The newSpans inUnsafetyandAsyncare then taken advantage of for better diagnostics. A reason this change was made is to have a more uniform and clear naming scheme.The HIR keeps the structures in AST (with those definitions moved into HIR) for now to avoid regressing perf.
Various cleanups, bug fixes, and diagnostics improvements are made along the way. It is probably best to understand those via the diffs.
I would recommend reviewing this commit-by-commit with whitespace changes hidden.
r? @estebank @petrochenkov