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.

Sign up for GitHub

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

Jump to bottom

Use full name to identify a macro in a FileName. #54338

Merged
merged 1 commit into from
Sep 28, 2018
Merged

Use full name to identify a macro in a FileName. #54338

merged 1 commit into from
Sep 28, 2018

Conversation

orium
Copy link
Member

@orium orium commented Sep 19, 2018

Before this two macros with same name would be indistinguishable inside a FileName. This caused a bug in incremental compilation (see #53097) since two different macros would map out to the same StableFilemapId.

Fixes #53097.

r? @nrc

@orium
Use full name to identify a macro in a FileName.
2d7edf9 
Before this two macros with same name would be indistinguishable inside a
`FileName`.  This caused a bug in incremental compilation (see rust-lang#53097) since
two different macros would map out to the same `StableFilemapId`.

Fixes rust-lang#53097.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2018
@TimNN
Copy link
Contributor

TimNN commented Sep 25, 2018

Ping from triage @nrc, this PR requires your review.

nrc
nrc reviewed Sep 26, 2018
View reviewed changes
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Code looks good, could you write a regression test for this?

@orium
Copy link
Member Author

orium commented Sep 26, 2018 •
edited
Loading

Code looks good, could you write a regression test for this?

The problem is that this is non-deterministic (probably the source of non-determinism is an hashtable somewhere, since they have a random seed). However I may be able to write a test that has a very high chance of failing.

@orium
Copy link
Member Author

orium commented Sep 26, 2018 •
edited
Loading

@nrc I tried a few things but I was unable to create a test that reproduced the bug. In fact the only way that I can reproduce it is by doing what I describe in #53097 (comment) (any change of the example I do would not trigger the bug).

@nrc
Copy link
Member

nrc commented Sep 28, 2018

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 28, 2018

📌 Commit 2d7edf9 has been approved by nrc

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2018
@bors
Copy link
Contributor

bors commented Sep 28, 2018

⌛ Testing commit 2d7edf9 with merge bd8d030...

bors added a commit that referenced this pull request Sep 28, 2018
@bors
Auto merge of #54338 - orium:fix-macro-inc-comp, r=nrc
bd8d030 
Use full name to identify a macro in a `FileName`.

Before this two macros with same name would be indistinguishable inside a `FileName`.  This caused a bug in incremental compilation (see #53097) since two different macros would map out to the same `StableFilemapId`.

Fixes #53097.

r? @nrc
@bors
Copy link
Contributor

bors commented Sep 28, 2018

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

@bors bors merged commit 2d7edf9 into rust-lang:master Sep 28, 2018
@orium orium deleted the fix-macro-inc-comp branch September 28, 2018 10:14
@orium orium added beta-nominated Nominated for backporting to the compiler in the beta channel. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 29, 2018
@pietroalbini pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 2, 2018
@oli-obk oli-obk added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Oct 2, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Oct 2, 2018

re-adding beta-nominated so that this will (hopefully) be enqueued for backport

@pnkfelix pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 2, 2018
@pietroalbini pietroalbini mentioned this pull request Oct 2, 2018
Merged
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 2, 2018
bors added a commit that referenced this pull request Oct 2, 2018
@bors
Auto merge of #54750 - pietroalbini:beta-backports, r=pietroalbini
3889530 
[beta] Rollup backports

Merged and approved:

* #54650: Don't lint non-extern-prelude extern crate's in Rust 2018.
* #54338: Use full name to identify a macro in a `FileName`.
* #54518: resolve: Do not block derive helper resolutions on single import resolutions
* #54581: Accept trailing comma in `cfg_attr`

r? @ghost
bors added a commit that referenced this pull request Oct 2, 2018
@bors
Auto merge of #54750 - pietroalbini:beta-backports, r=pietroalbini
87e1957 
[beta] Rollup backports

Merged and approved:

* #54650: Don't lint non-extern-prelude extern crate's in Rust 2018.
* #54338: Use full name to identify a macro in a `FileName`.
* #54518: resolve: Do not block derive helper resolutions on single import resolutions
* #54581: Accept trailing comma in `cfg_attr`

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrc nrc nrc left review comments

Assignees

@nrc nrc

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@orium @rust-highfive @TimNN @nrc @bors @pnkfelix @oli-obk @pietroalbini