Resolve attributes in several places #63468

c410-f3r · 2019-08-11T18:44:09Z

Resolve attributes for Arm, Field, FieldPat, GenericParam, Param, StructField and Variant.

This PR is based on @petrochenkov work located at petrochenkov@83fdb8d.

petrochenkov · 2019-08-11T18:49:18Z

Meta: "draft" PRs don't interact well with our tooling, so it's better to avoid them.
"WIP" in the title is enough to see that the PR is an in progress work.

src/librustc_typeck/collect.rs

src/libsyntax/ast.rs

src/libsyntax/ext/base.rs

src/librustc/hir/mod.rs

src/libsyntax/ext/base.rs

src/libsyntax/visit.rs

petrochenkov · 2019-08-12T13:44:45Z

@c410-f3r
You can move the generic AST/HIR changes (Variant, new NodeIds) and visitor/mut_visitor changes into a separate PR, then we'll be able to merge them quickly.

src/librustc/hir/map/def_collector.rs

Centril · 2019-08-12T13:53:33Z

(We'll need to feature gate using proc macro attrs on the new forms as well and set up a tracking issue unless there already is one.)

petrochenkov · 2019-08-12T14:02:10Z

@Centril
The "expected an inert attribute, found an attribute macro" error in resolve currently works as a "feature gate" that cannot be enabled.

Centril · 2019-08-12T14:14:07Z

@petrochenkov I see; I was confused by the added // check-pass test (which shouldn't pass...).

c410-f3r · 2019-08-12T16:49:01Z

You can move the generic AST/HIR changes (Variant, new NodeIds) and visitor/mut_visitor changes into a separate PR, then we'll be able to merge them quickly.

Indeed

src/librustc_resolve/build_reduced_graph.rs

src/libsyntax/ext/expand.rs

src/libsyntax/ext/base.rs

@estebank

syntax: Remove `DummyResult::expr_only` The effect is that if a built-in macro both returns an erroneous AST fragment and is used in unexpected position, then the incorrect position error won't be reported. This combination of two errors should be rare and isn't worth an extra field that makes people ask questions in comments. (There wasn't even a test making sure it worked.) Addresses rust-lang#63468 (comment) r? @estebank

Add NodeId for Arm, Field and FieldPat Extracted from rust-lang#63468

@estebank

syntax: Remove `DummyResult::expr_only` The effect is that if a built-in macro both returns an erroneous AST fragment and is used in unexpected position, then the incorrect position error won't be reported. This combination of two errors should be rare and isn't worth an extra field that makes people ask questions in comments. (There wasn't even a test making sure it worked.) Addresses rust-lang#63468 (comment) r? @estebank

expand: Unimplement `MutVisitor` on `MacroExpander` Each call to `fully_expand_fragment` is something unique, interesting, and requiring attention. It represents a "root" of expansion and its use means that something unusual is happening, like eager expansion or expansion performed outside of the primary expansion pass. So, it shouldn't hide under a generic visitor call. Also, from all the implemented visitor methods only two were actually used. cc rust-lang#63468 (comment)

Add NodeId for Arm, Field and FieldPat Extracted from rust-lang#63468

@estebank

syntax: Remove `DummyResult::expr_only` The effect is that if a built-in macro both returns an erroneous AST fragment and is used in unexpected position, then the incorrect position error won't be reported. This combination of two errors should be rare and isn't worth an extra field that makes people ask questions in comments. (There wasn't even a test making sure it worked.) Addresses rust-lang#63468 (comment) r? @estebank

c410-f3r · 2019-09-09T11:56:53Z

@rustbot modify labels to +S-waiting-on-review , -S-waiting-on-author

src/test/ui/attrs-resolution-errors.rs

src/test/ui/attrs-resolution.rs

src/librustc_resolve/lib.rs

src/librustc_resolve/build_reduced_graph.rs

petrochenkov · 2019-09-09T12:12:38Z

Great!
r=me with the remaining nits addressed and green CI

Arm, Field, FieldPat, GenericParam, Param, StructField and Variant

c410-f3r · 2019-09-09T12:28:22Z

@petrochenkov Thanks for your patience

Centril · 2019-09-09T15:29:20Z

@bors r=petrochenkov

bors · 2019-09-09T15:29:21Z

📌 Commit 63a5f39 has been approved by petrochenkov

@petrochenkov

Resolve attributes in several places Resolve attributes for Arm, Field, FieldPat, GenericParam, Param, StructField and Variant. This PR is based on @petrochenkov work located at petrochenkov@83fdb8d.

@ghost

Rollup of 5 pull requests Successful merges: - #63468 (Resolve attributes in several places) - #64121 (Override `StepBy::{try_fold, try_rfold}`) - #64278 (check git in bootstrap.py) - #64306 (Fix typo in config.toml.example) - #64312 (Unify escape usage) Failed merges: r? @ghost

petrochenkov · 2019-09-09T20:46:23Z

@c410-f3r
Thanks a lot, this was a longstanding issue and the last major missing part of macro modularization.

feature_gate: Remove dead code from attribute checking rust-lang#63468 is merged, so all attributes go through name resolution now, so we can remove code that previously performed some checks for attributes not going through resolution.

Fully integrate derive helpers into name resolution ```rust #[derive(Foo)] #[foo_helper] // already goes through name resolution struct S { #[foo_helper] // goes through name resolution after this PR field: u8 } ``` How name resolution normally works: - We have an identifier we need to resolve, take its location (in some sense) and look what names are in scope in that location. How derive helper attributes are "resolved" (before this PR): - After resolving the derive `Foo` we visit the derive's input (`struct S { ... } `) as a piece of AST and mark attributes textually matching one of the derive's helper attributes (`foo_helper`) as "known", so they never go through name resolution. This PR changes the rules for derive helpers, so they are not proactively marked as known (which is a big hack ignoring any ambiguities or hygiene), but go through regular name resolution instead. This change was previously blocked by attributes not being resolved in some positions at all (fixed in #63468). "Where exactly are derive helpers in scope?" is an interesting question, and I need some feedback from proc macro authors to answer it, see the post below (#64694 (comment)).

Starting from rust-lang#63468 cfg attributes on variants, fields, fn params etc. are processed together with other attributes (via `configure!`).

rust-highfive assigned petrochenkov Aug 11, 2019

This comment has been minimized.

Sign in to view

c410-f3r changed the title ~~Resolve attributes in several places~~ [WIP] Resolve attributes in several places Aug 11, 2019

c410-f3r marked this pull request as ready for review August 11, 2019 19:19

Centril reviewed Aug 11, 2019

View reviewed changes

src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved

src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved

src/libsyntax/ast.rs Outdated Show resolved Hide resolved

src/libsyntax/ext/base.rs Outdated Show resolved Hide resolved

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2019

This comment has been minimized.

Sign in to view

petrochenkov reviewed Aug 12, 2019

View reviewed changes

src/librustc/hir/mod.rs Outdated Show resolved Hide resolved

src/libsyntax/ext/base.rs Outdated Show resolved Hide resolved

src/libsyntax/visit.rs Outdated Show resolved Hide resolved

petrochenkov reviewed Aug 12, 2019

View reviewed changes

src/librustc/hir/map/def_collector.rs Outdated Show resolved Hide resolved

petrochenkov reviewed Aug 13, 2019

View reviewed changes

src/librustc_resolve/build_reduced_graph.rs Outdated Show resolved Hide resolved

petrochenkov reviewed Aug 13, 2019

View reviewed changes

src/libsyntax/ext/expand.rs Show resolved Hide resolved

petrochenkov reviewed Aug 13, 2019

View reviewed changes

src/libsyntax/ext/base.rs Outdated Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2019

This was referenced Aug 13, 2019

syntax: Remove DummyResult::expr_only #63528

Merged

expand: Unimplement MutVisitor on MacroExpander #63537

Merged

This was referenced Aug 14, 2019

Add NodeId for Arm, Field and FieldPat #63542

Merged

Merge Variant and Variant_ #63543

Merged

Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019

Rollup merge of rust-lang#63542 - c410-f3r:node_ids, r=petrochenkov

82f2dad

Add NodeId for Arm, Field and FieldPat Extracted from rust-lang#63468

Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019

Rollup merge of rust-lang#63542 - c410-f3r:node_ids, r=petrochenkov

2c8bf8e

Add NodeId for Arm, Field and FieldPat Extracted from rust-lang#63468

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 9, 2019

petrochenkov reviewed Sep 9, 2019

View reviewed changes

src/test/ui/attrs-resolution-errors.rs Outdated Show resolved Hide resolved

src/test/ui/attrs-resolution.rs Outdated Show resolved Hide resolved

src/librustc_resolve/lib.rs Show resolved Hide resolved

src/librustc_resolve/build_reduced_graph.rs Outdated Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2019

c410-f3r force-pushed the attrs branch from 003455e to df702ab Compare September 9, 2019 12:26

Resolve attributes in several places

63a5f39

Arm, Field, FieldPat, GenericParam, Param, StructField and Variant

c410-f3r force-pushed the attrs branch from df702ab to 63a5f39 Compare September 9, 2019 12:28

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 9, 2019

Centril mentioned this pull request Sep 9, 2019

Rollup of 5 pull requests #64313

Merged

bors merged commit 63a5f39 into rust-lang:master Sep 9, 2019

dtolnay mentioned this pull request Sep 11, 2019

"Don't know the field number in this context" #64385

Closed

petrochenkov mentioned this pull request Sep 14, 2019

feature_gate: Remove dead code from attribute checking #64462

Merged

petrochenkov mentioned this pull request Sep 22, 2019

Fully integrate derive helpers into name resolution #64694

Merged

petrochenkov mentioned this pull request Oct 17, 2019

derive structs (inert attribute?) regression 1.39 #65521

Closed

This was referenced Nov 19, 2019

ICE 'no entry found for key' in Servo’s style crate #66487

Closed

tuple structs with all-public fields can not be instantiated if one of the fields has an external attribute #66555

Closed

petrochenkov mentioned this pull request Dec 6, 2019

attempted .def_id() on invalid res: Err in rustdoc for pub(restricted) struct fields using derive helper macros #67006

Closed

petrochenkov added a commit to petrochenkov/rust that referenced this pull request Feb 14, 2021

expand: Remove redundant calls to configure

6e11a8b

Starting from rust-lang#63468 cfg attributes on variants, fields, fn params etc. are processed together with other attributes (via `configure!`).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve attributes in several places #63468

Resolve attributes in several places #63468

c410-f3r commented Aug 11, 2019 •

edited

Loading

petrochenkov commented Aug 11, 2019

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Aug 12, 2019 •

edited

Loading

Centril commented Aug 12, 2019

petrochenkov commented Aug 12, 2019

Centril commented Aug 12, 2019 •

edited

Loading

c410-f3r commented Aug 12, 2019

c410-f3r commented Sep 9, 2019

petrochenkov commented Sep 9, 2019

c410-f3r commented Sep 9, 2019 •

edited

Loading

Centril commented Sep 9, 2019

bors commented Sep 9, 2019

petrochenkov commented Sep 9, 2019

Resolve attributes in several places #63468

Resolve attributes in several places #63468

Conversation

c410-f3r commented Aug 11, 2019 • edited Loading

petrochenkov commented Aug 11, 2019

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Aug 12, 2019 • edited Loading

Centril commented Aug 12, 2019

petrochenkov commented Aug 12, 2019

Centril commented Aug 12, 2019 • edited Loading

c410-f3r commented Aug 12, 2019

c410-f3r commented Sep 9, 2019

petrochenkov commented Sep 9, 2019

c410-f3r commented Sep 9, 2019 • edited Loading

Centril commented Sep 9, 2019

bors commented Sep 9, 2019

petrochenkov commented Sep 9, 2019

c410-f3r commented Aug 11, 2019 •

edited

Loading

petrochenkov commented Aug 12, 2019 •

edited

Loading

Centril commented Aug 12, 2019 •

edited

Loading

c410-f3r commented Sep 9, 2019 •

edited

Loading