Skip to content

Conversation

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Aug 25, 2018

At least the incremental compilation code, and a few other places in the
compiler, require the CrateMetadata for a loaded target crate to contain a
valid DefIdTable for the DefIds in the target.

Previously, the CrateMetadata for a proc macro contained the crate's
"host" DefIdTable, which is of course incompatible with the "target"
DefIdTable, causing ICEs. This creates a DefIdTable that properly refers
to the "proc macro" DefIds.

Fixes #49482.

r? @michaelwoerister

Should we beta-nominate this?

@arielb1
Copy link
Contributor Author

arielb1 commented Aug 25, 2018

r? @michaelwoerister

At least the incremental compilation code, and a few other places in the
compiler, require the CrateMetadata for a loaded target crate to contain a
valid DefIdTable for the DefIds in the target.

Previously, the CrateMetadata for a proc macro contained the crate's
"host" DefIdTable, which is of course incompatible with the "target"
DefIdTable, causing ICEs. This creates a DefIdTable that properly refers
to the "proc macro" DefIds.

Fixes rust-lang#49482.
@michaelwoerister
Copy link
Member

Thanks a lot, @arielb1!

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 27, 2018

📌 Commit 025d014 has been approved by michaelwoerister

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 27, 2018
@michaelwoerister michaelwoerister added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 27, 2018
@bors
Copy link
Collaborator

bors commented Aug 29, 2018

⌛ Testing commit 025d014 with merge ca0de63...

bors added a commit that referenced this pull request Aug 29, 2018
create a valid DefIdTable for proc macro crates

At least the incremental compilation code, and a few other places in the
compiler, require the CrateMetadata for a loaded target crate to contain a
valid DefIdTable for the DefIds in the target.

Previously, the CrateMetadata for a proc macro contained the crate's
"host" DefIdTable, which is of course incompatible with the "target"
DefIdTable, causing ICEs. This creates a DefIdTable that properly refers
to the "proc macro" DefIds.

Fixes #49482.

r? @michaelwoerister

Should we beta-nominate this?
@bors
Copy link
Collaborator

bors commented Aug 29, 2018

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

@bors bors merged commit 025d014 into rust-lang:master Aug 29, 2018
@pietroalbini pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 30, 2018
@nikomatsakis
Copy link
Contributor

cc @rust-lang/compiler

I'm torn on whether to backport this. The fix is a bit large — I don't have a good read on our "confidence level" that it is correct. I guess a worthy question is how much the people who filed #49482 care if it is backported. I'll ask on the issue :)

@glebpom
Copy link

glebpom commented Sep 4, 2018

@nikomatsakis Regarding the possible backporting: I generally fixed this issue in my code by dropping some of the dependencies with a bit of code rework. As soon as there are few users affected by the issue, I think there is no strong need to backport this to beta.

@pnkfelix
Copy link
Contributor

pnkfelix commented Sep 6, 2018

discussed in compiler team meeting. Team decided PR is too big, and the bug sufficiently low rsk, that we will not backport to beta.

@pnkfelix pnkfelix removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants