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

resolve: Relax one restriction on macro namespace #56573

Closed
wants to merge 1 commit into from

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Dec 6, 2018

The restriction is "macro-expanded macros cannot shadow glob-imported macros during resolution in module", where "in module" means the macro path has module prefix (module::my_macro!(), but not my_macro!()).

This is a special case that was assumed to be necessary to resolve multi-segment macro paths reliably, but it's likely that the assumption was incorrect.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2018
@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2018
@petrochenkov
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Dec 6, 2018

⌛ Trying commit de6eadb with merge 1e86cd8...

bors added a commit that referenced this pull request Dec 6, 2018
resolve: Relax one restriction on macro namespace

It was assumed to be necessary to resolve multi-segment macro paths reliably, but it's likely that the assumption was incorrect.
@bors
Copy link
Contributor

bors commented Dec 6, 2018

☀️ Test successful - status-travis
State: approved= try=True

@petrochenkov
Copy link
Contributor Author

@craterbot run start=master#cd48ce1e9a9c0965f01ede2ea1a554eca2c74336 end=try#1e86cd8e8fc3d50d814cefa5dbd42a13b09d4e22 mode=check-only

@craterbot
Copy link
Collaborator

👌 Experiment pr-56573 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-56573 is now running on agent aws-2.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Centril
Copy link
Contributor

Centril commented Dec 8, 2018

Can you expand on why we are doing this change and why "it's likely that the assumption was incorrect."?

@craterbot
Copy link
Collaborator

🎉 Experiment pr-56573 is completed!
📊 0 regressed and 1 fixed (50140 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 8, 2018
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Dec 9, 2018

@Centril

why "it's likely that the assumption was incorrect."?

Imports are also resolved early, as macros, but don't have this restriction.
So far we didn't have problems with resolving imports (*), so it's unlikely we'll have problems with applying same rules for macros (as crater shows).

* However, one timely reported issue (#56593) shows that we did have problems with imports all this time in the same situation (macro expanded name shadows glob import), so perhaps we need to keep the restriction for macros and additionally apply it imports as well.

I'll close this until the situation with #56593 becomes clear.

@petrochenkov petrochenkov deleted the macrel branch June 5, 2019 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants