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

Improve resolution of associated types in declarative macros 2.0 #44818

Merged
merged 1 commit into from
Oct 6, 2017

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Sep 24, 2017

Make various identifier comparisons for associated types (and sometimes other associated items) hygienic.
Now declarative macros 2.0 can use Self::AssocTy, TyParam::AssocTy, Trait<AssocTy = u8> where AssocTy is an associated type of a trait Trait visible from the macro. Also, Trait can now be implemented inside the macro and specialization should work properly (fixes #40847 (comment)).

r? @jseyfried or @eddyb

@eddyb
Copy link
Member

eddyb commented Sep 25, 2017

cc @nikomatsakis

-> impl Iterator<Item = NodeItem<ty::AssociatedItem>> + 'a {
self.flat_map(move |node| {
node.items(tcx).filter(move |item| {
let item_ident = tcx.adjust(item.name, trait_def_id, impl_node_id).0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, actually adjust doesn't require the NodeId if we need only the adjusted ident, so it looks like specialization can be fixed as well.

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2017
@petrochenkov
Copy link
Contributor Author

Updated with a fix for specialization.

There are still plenty of unhygienic comparisons to fix (see loops through tcx.associated_items(trait_def_id)), but I'll be mostly away from computer for the next few days.

Copy link
Contributor

@jseyfried jseyfried left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!
r=me modulo optional comment.

@@ -2282,6 +2282,13 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
}
}

// Hygienically compare a use-site name (`use_name`) for a field or an associated item with its
// supposed definition name (`def_name`). The method also needs the supposed definition's
// `DefId` to perform comparison.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def_def_id "should" be the DefId of supposed definition's parent, i.e. the scope (module, enum, struct, trait, or impl) in which we are resolving use_name (the name of the the item, variant, field, trait item, or impl item, resp.).

However, I believe this distinction only makes a difference when resolving in modules, so we can probably safely use the semantics in this PR for resolution that happens after resolve.

@jseyfried
Copy link
Contributor

cc @nrc

@carols10cents carols10cents 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 Oct 2, 2017
@petrochenkov
Copy link
Contributor Author

Rebased + used trait DefIds in accordance with #44818 (comment)
@bors r=jseyfried

@bors
Copy link
Contributor

bors commented Oct 5, 2017

📌 Commit 2d9161d has been approved by jseyfried

@bors
Copy link
Contributor

bors commented Oct 6, 2017

⌛ Testing commit 2d9161d with merge 7b54cd2...

bors added a commit that referenced this pull request Oct 6, 2017
Improve resolution of associated types in declarative macros 2.0

Make various identifier comparisons for associated types (and sometimes other associated items) hygienic.
Now declarative macros 2.0 can use `Self::AssocTy`, `TyParam::AssocTy`, `Trait<AssocTy = u8>` where `AssocTy` is an associated type of a trait `Trait` visible from the macro. Also, `Trait` can now be implemented inside the macro and specialization should work properly (fixes #40847 (comment)).

r? @jseyfried or @eddyb
@bors
Copy link
Contributor

bors commented Oct 6, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Oct 6, 2017

@bors retry (maybe #43402)

i686-gnu failed at issue-26092. cc @aidanhs

[01:21:25] ---- [run-make] run-make/issue-26092 stdout ----
[01:21:25] 	
[01:21:25] error: make failed
[01:21:25] status: exit code: 2
[01:21:25] command: "make"
[01:21:25] stdout:
[01:21:25] ------------------------------------------
[01:21:25] LD_LIBRARY_PATH="/checkout/obj/build/i686-unknown-linux-gnu/test/run-make/issue-26092.stage2-i686-unknown-linux-gnu:/checkout/obj/build/i686-unknown-linux-gnu/stage2/lib:/checkout/obj/build/i686-unknown-linux-gnu/stage0-tools/i686-unknown-linux-gnu/release/deps:/checkout/obj/build/i686-unknown-linux-gnu/stage0-sysroot/lib/rustlib/i686-unknown-linux-gnu/lib:" '/checkout/obj/build/i686-unknown-linux-gnu/stage2/bin/rustc' --out-dir /checkout/obj/build/i686-unknown-linux-gnu/test/run-make/issue-26092.stage2-i686-unknown-linux-gnu -L /checkout/obj/build/i686-unknown-linux-gnu/test/run-make/issue-26092.stage2-i686-unknown-linux-gnu  -o "" blank.rs 2>&1 | \
[01:21:25] 		grep -i 'No such file or directory'
[01:21:25] Makefile:4: recipe for target 'all' failed
[01:21:25] 
[01:21:25] ------------------------------------------
[01:21:25] stderr:
[01:21:25] ------------------------------------------
[01:21:25] make: *** [all] Error 1
[01:21:25] 
[01:21:25] ------------------------------------------
[01:21:25] 
[01:21:25] thread '[run-make] run-make/issue-26092' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2433:8
[01:21:25] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:21:25] 
[01:21:25] 
[01:21:25] failures:
[01:21:25]     [run-make] run-make/issue-26092
[01:21:25] 
[01:21:25] test result: �[31mFAILED�(B�[m. 160 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
[01:21:25] 
[01:21:25] thread 'main' panicked at 'Some tests failed', /checkout/src/tools/compiletest/src/main.rs:323:21

@bors
Copy link
Contributor

bors commented Oct 6, 2017

⌛ Testing commit 2d9161d with merge ed1cffd...

bors added a commit that referenced this pull request Oct 6, 2017
Improve resolution of associated types in declarative macros 2.0

Make various identifier comparisons for associated types (and sometimes other associated items) hygienic.
Now declarative macros 2.0 can use `Self::AssocTy`, `TyParam::AssocTy`, `Trait<AssocTy = u8>` where `AssocTy` is an associated type of a trait `Trait` visible from the macro. Also, `Trait` can now be implemented inside the macro and specialization should work properly (fixes #40847 (comment)).

r? @jseyfried or @eddyb
@bors
Copy link
Contributor

bors commented Oct 6, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: jseyfried
Pushing ed1cffd to master...

@bors bors merged commit 2d9161d into rust-lang:master Oct 6, 2017
@petrochenkov petrochenkov deleted the astymac2 branch June 5, 2019 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants