Skip to content
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

Fully integrate derive helpers into name resolution #64694

Merged
merged 4 commits into from Nov 16, 2019

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 22, 2019

#[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)).

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Sep 22, 2019

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Sep 22, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Sep 22, 2019

The current rules ("walk the derive's input AST and mark its helpers as known") works like this:

#[derive(Foo)]
struct S {
    #[foo_helper] // marked as known
    field1: u8,
    #[some_attr]
    #[foo_helper] // marked as known
    field2: u8,
    field3: bang_macro_type!(), // `#[foo_helper]` inside the macro is NOT marked as known
}

// Generated by `Foo`
impl Foo for S {
    #[derive_helper] // NOT marked as known
    fn method(&self) { /* ... */ }
}

If we reformulate it in terms of scopes, then we get something like this:

  • The helper is in scope directly inside the expansion of #[derive(Foo)] (e.g. in #[foo_helper] field1).
  • The helper is in scope inside nested attribute and derive expansions inside the expansion of #[derive(Foo)] (e.g. in #[foo_helper] field2 which is inside the expansion of #[some_attr]).
  • The helper is NOT in scope inside nested bang macro expansion inside the expansion of #[derive(Foo)] (e.g. in bang_macro_type!())
  • The helper is NOT in scope inside the expansion of Foo (the generated code, i.e. impl Foo for S { ... }), which is different from expansion of #[derive(Foo)] which includes non-generated struct S.

The question is - how close should we mirror these rules?

Can the rule be just "The helper is in scope inside the expansion of #[derive(Foo)]", without the "NOT"s? This is useless for derives, but I think it may be useful if we start supporting helpers for attribute macros, in addition to derive macros:

#[foo_attr] // #[proc_macro_attribute(attributes(foo_helper))] fn foo_attr(...) { ... }
struct S {
    // no need to erase this attribute from code generated by `foo_attr`,
    // it will be treated as a known helper attribute
    #[foo_helper]
    field: u8,
}

What do you think?

cc @dtolnay @alexcrichton @SergioBenitez

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Sep 22, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-09-22T22:16:10.8634016Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-22T22:16:10.8850634Z ##[command]git config gc.auto 0
2019-09-22T22:16:10.8927794Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-22T22:16:10.8970735Z ##[command]git config --get-all http.proxy
2019-09-22T22:16:10.9099979Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64694/merge:refs/remotes/pull/64694/merge
---
2019-09-22T23:14:52.4579324Z .................................................................................................... 1500/9035
2019-09-22T23:14:58.5196481Z .................................................................................................... 1600/9035
2019-09-22T23:15:10.7578333Z ........................................................................i...............i........... 1700/9035
2019-09-22T23:15:17.5120198Z .................................................................................................... 1800/9035
2019-09-22T23:15:26.2271165Z ...............................................................iiiii................................ 1900/9035
2019-09-22T23:15:45.1323602Z .................................................................................................... 2100/9035
2019-09-22T23:15:47.5813478Z .................................................................................................... 2200/9035
2019-09-22T23:15:50.7635092Z .................................................................................................... 2300/9035
2019-09-22T23:15:59.1821935Z .................................................................................................... 2400/9035
---
2019-09-22T23:19:01.4048625Z ....................................................i..............i................................ 4700/9035
2019-09-22T23:19:10.9149173Z .................................................................................................... 4800/9035
2019-09-22T23:19:19.6422375Z .................................................................................................... 4900/9035
2019-09-22T23:19:27.4545698Z .................................................................................................... 5000/9035
2019-09-22T23:19:37.4332695Z ......................................ii.ii......................................................... 5100/9035
2019-09-22T23:19:47.5999189Z .................................................................................................... 5300/9035
2019-09-22T23:19:58.4947882Z .................................................................................................... 5400/9035
2019-09-22T23:20:06.3421862Z ...i................................................................................................ 5500/9035
2019-09-22T23:20:11.8861398Z .................................................................................................... 5600/9035
2019-09-22T23:20:11.8861398Z .................................................................................................... 5600/9035
2019-09-22T23:20:23.7942093Z ..................................................................................................ii 5700/9035
2019-09-22T23:20:37.7232023Z ...i..ii...........i................................................................................ 5800/9035
2019-09-22T23:21:00.1582859Z .................................................................................................... 6000/9035
2019-09-22T23:21:06.7565211Z .................................................................................................... 6100/9035
2019-09-22T23:21:06.7565211Z .................................................................................................... 6100/9035
2019-09-22T23:21:21.0696863Z i..ii............................................................................................... 6200/9035
2019-09-22T23:21:40.0807025Z ...........................................................i........................................ 6400/9035
2019-09-22T23:21:42.2848888Z .................................................................................................... 6500/9035
2019-09-22T23:21:44.8455687Z ...............................i.................................................................... 6600/9035
2019-09-22T23:21:49.0698388Z .................................................................................................... 6700/9035
---
2019-09-22T23:26:21.6417478Z  finished in 5.425
2019-09-22T23:26:21.6609180Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-22T23:26:21.8466813Z 
2019-09-22T23:26:21.8467263Z running 150 tests
2019-09-22T23:26:25.2723164Z i....iii......iii..iiii....i.............................i..i..................i....i.........ii.i.i 100/150
2019-09-22T23:26:27.2691593Z ..iiii..............i.........iii.i.......ii......
2019-09-22T23:26:27.2692175Z 
2019-09-22T23:26:27.2692330Z  finished in 5.608
2019-09-22T23:26:27.2887877Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-22T23:26:27.4540221Z 
---
2019-09-22T23:26:29.5595587Z  finished in 2.270
2019-09-22T23:26:29.5800721Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-22T23:26:29.7377909Z 
2019-09-22T23:26:29.7378692Z running 9 tests
2019-09-22T23:26:29.7381003Z iiiiiiiii
2019-09-22T23:26:29.7381850Z 
2019-09-22T23:26:29.7386325Z  finished in 0.158
2019-09-22T23:26:29.7598963Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-22T23:26:29.9400709Z 
---
2019-09-22T23:26:48.3844662Z  finished in 18.624
2019-09-22T23:26:48.4083700Z Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-22T23:26:48.5924008Z 
2019-09-22T23:26:48.5924349Z running 123 tests
2019-09-22T23:27:12.7292345Z .iiiii...i.....i..i...i..i.i.i..i.ii..i.i.....i..i....ii..........iiii..........i...ii...i.......ii. 100/123
2019-09-22T23:27:17.3164855Z i.i.i......iii.i.....ii
2019-09-22T23:27:17.3167603Z 
2019-09-22T23:27:17.3169544Z  finished in 28.908
2019-09-22T23:27:17.3218354Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-22T23:27:17.3218678Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2019-09-22T23:28:17.6109151Z ---- [ui] ui-fulldeps/plugin-attr-register-deny.rs stdout ----
2019-09-22T23:28:17.6109488Z 
2019-09-22T23:28:17.6109615Z error: ui test compiled successfully!
2019-09-22T23:28:17.6109736Z status: exit code: 0
2019-09-22T23:28:17.6110874Z command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui-fulldeps/plugin-attr-register-deny.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/plugin-attr-register-deny" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/plugin-attr-register-deny/auxiliary" "-A" "unused"
2019-09-22T23:28:17.6112044Z ------------------------------------------
2019-09-22T23:28:17.6112230Z 
2019-09-22T23:28:17.6112631Z ------------------------------------------
2019-09-22T23:28:17.6112822Z stderr:
---
2019-09-22T23:28:17.6115411Z test result: FAILED. 68 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
2019-09-22T23:28:17.6115633Z 
2019-09-22T23:28:17.6115915Z 
2019-09-22T23:28:17.6116034Z 
2019-09-22T23:28:17.6117545Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui-fulldeps" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
2019-09-22T23:28:17.6155687Z 
2019-09-22T23:28:17.6155968Z 
2019-09-22T23:28:17.6156258Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-09-22T23:28:17.6156407Z Build completed unsuccessfully in 1:05:11
2019-09-22T23:28:17.6156407Z Build completed unsuccessfully in 1:05:11
2019-09-22T23:28:17.6182968Z == clock drift check ==
2019-09-22T23:28:17.6199515Z   local time: Sun Sep 22 23:28:17 UTC 2019
2019-09-22T23:28:17.8996362Z   network time: Sun, 22 Sep 2019 23:28:17 GMT
2019-09-22T23:28:17.8996517Z == end clock drift check ==
2019-09-22T23:28:18.7936575Z ##[error]Bash exited with code '1'.
2019-09-22T23:28:18.7970176Z ##[section]Starting: Checkout
2019-09-22T23:28:18.7972505Z ==============================================================================
2019-09-22T23:28:18.7972563Z Task         : Get sources
2019-09-22T23:28:18.7972627Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

field: [u8; {
// FIXME No ambiguity, derive helpers are not put into scope for non-attributes
use empty_helper;
use empty_helper; //~ ERROR `empty_helper` is ambiguous

This comment has been minimized.

Copy link
@dtolnay

dtolnay Sep 23, 2019

Member

If this were use crate::empty_helper, is the attribute on struct U still ambiguous?

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Sep 23, 2019

Author Contributor

The attribute on struct U is still ambiguous because two different empty_helpers still exist in scope at the struct U point, and derive helpers always produce an error in case of ambiguity (shadowing is not allowed).

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Sep 23, 2019

The new behavior seems reasonable to me from reading derive-helper-shadowing.rs.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 23, 2019

This all seems reasonable to me!

@SergioBenitez

This comment has been minimized.

Copy link
Contributor

SergioBenitez commented Sep 26, 2019

This sounds good to me. I think the test should include a use of #[empty_helper] outside of the derive expansion of struct S; that application should unambiguously resolve to empty_attr.

Also, since this is a breaking change, how will it be rolled out?

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Sep 26, 2019

Also, since this is a breaking change, how will it be rolled out?

I planned to just land this as is for now.
We have regular crater runs for every release and 12 weeks for people to report issues, if some regressions come up I'll prepare some targeted deprecation warning for them, or relax the "ambiguity is always an error" rule and make derive helpers have highest priority.

@Centril Centril added the I-nominated label Sep 26, 2019
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Sep 26, 2019

r? @pnkfelix who will look at this from a language team perspective. We did not have time to fully process the PR in this meeting.

@rust-highfive rust-highfive assigned pnkfelix and unassigned estebank Sep 26, 2019
@petrochenkov petrochenkov force-pushed the petrochenkov:reshelp branch from 0137406 to 53a90e4 Oct 5, 2019
@petrochenkov petrochenkov changed the title [WIP] Fully integrate derive helpers into name resolution Fully integrate derive helpers into name resolution Oct 5, 2019
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Oct 5, 2019

Updated with a complete implementation and more tests.

The implementation now mirrors existing rules very closely, the helpers are not visible inside code expanded from non-attribute macros.

#[derive(Trait)]
struct S {
    field: type!(), // code inside `type!()` doesn't see the Trait's helpers
}

// Generated by the derive above
impl Trait for S { ... } // code in the generated impl also doesn't see the Trait's helpers
@joelpalmer

This comment has been minimized.

Copy link

joelpalmer commented Oct 14, 2019

Ping from Triage: @SergioBenitez @dtolnay -- any updated review for @petrochenkov's last comment?

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Oct 17, 2019

@petrochenkov I've reviewed your comments here, the implementation, and (perhaps most importantly), the additions you have made to src/test/ui/proc-macro/derive-helper-shadowing.rs

I have two main observations that I want to double-check with you about the test:

  1. the vast majority of additions to the test are demonstating cases that were always meant to be treated as erroneous but were not due to compiler bugs that you are fixing here. (Many of these were pre-existing in the test file, with no //~ ERROR annotation, and marked "FIXME".) The errors that are signalled are either ambiguity-errors (where there are two different empty_helpers in scope) or unresolvable-errors (where there is no empty_helper in scope). So far, so good; all the cases of this seem reasonable to me. (But of course they are all potential breaking-changes to be wary of, as previously discussed on this PR.)

  2. There are two points where the test is showing cases that are not signaled as errors by the compiler. There are both marked with comments that say "// OK, no ambiguity, ..." and then an explanation as to why its unambiguous.

My question is: In this latter case, illustrated by the two points marked "// OK, no ambiguity", where the attribute is resolvable: Is the observable behavior the same or potentially different from what we would have observed before this PR? Is it possible that some code exists where a procedural macro successfully expands some code without error both before and after the effects of this PR, but the semantics of the expansion have silently changed? Or is it not possible for this to happen?

(My current guess is that any client who was using helper or non-helper attributes in a non-erroneous manner before, and in a manner such that it continues to not error now, must have been using them in a manner that will not be affected by this change.)

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Oct 17, 2019

@pnkfelix

Is it possible that some code exists where a procedural macro successfully expands some code without error both before and after the effects of this PR, but the semantics of the expansion have silently changed? Or is it not possible for this to happen?

No, this shouldn't be possible.
The possible changes are:

  • Some code reports an ambiguity error where it previously compiled.
  • Some code compiles where it previously didn't (use derive_helper; on 2018 edition due to uniform paths).
@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Oct 17, 2019

@rust-lang/lang I think we can move forward with this

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Oct 17, 2019

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Oct 17, 2019

Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

petrochenkov added 4 commits Oct 3, 2019
These helpers are resolved before their respective derives through a kind of look ahead into future expansions.
Some of these will migrate to proper resolution, others will be deprecated.

```
#[trait_helper] // Deprecate
#[derive(Trait)]
#[trait_helper] // Migrate to proper resolution
```
Pass them through name resolution instead
@petrochenkov petrochenkov force-pushed the petrochenkov:reshelp branch from 53a90e4 to 8668c1a Nov 16, 2019
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 16, 2019

Rebased.
r? @matthewjasper

@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Nov 16, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 16, 2019

📌 Commit 8668c1a has been approved by matthewjasper

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 16, 2019

⌛️ Testing commit 8668c1a with merge 5c5b8af...

bors added a commit that referenced this pull request Nov 16, 2019
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)).
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 16, 2019

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing 5c5b8af to master...

@bors bors added the merged-by-bors label Nov 16, 2019
@bors bors merged commit 8668c1a into rust-lang:master Nov 16, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191116.29 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 19, 2019
resolve: Give derive helpers highest priority during resolution

So they just shadow everything else and don't create ambiguity errors.
This matches the old pre-rust-lang#64694 behavior most closely.

---
The change doesn't apply to this "compatibility" case
```rust
#[trait_helper] // The helper attribute is used before it introduced.
                        // Sadly, compiles on stable, supported via hacks.
                        // I plan to make a compatibility warning for this.
#[derive(Trait)]
struct S;
```
, such attributes still create ambiguities, but rust-lang#64694 didn't change anything for this case.

Fixes rust-lang#66508
Fixes rust-lang#66525
Centril added a commit to Centril/rust that referenced this pull request Nov 19, 2019
resolve: Give derive helpers highest priority during resolution

So they just shadow everything else and don't create ambiguity errors.
This matches the old pre-rust-lang#64694 behavior most closely.

---
The change doesn't apply to this "compatibility" case
```rust
#[trait_helper] // The helper attribute is used before it introduced.
                        // Sadly, compiles on stable, supported via hacks.
                        // I plan to make a compatibility warning for this.
#[derive(Trait)]
struct S;
```
, such attributes still create ambiguities, but rust-lang#64694 didn't change anything for this case.

Fixes rust-lang#66508
Fixes rust-lang#66525
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.