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

loosen assertion against proj in collector #36876

Merged
merged 1 commit into from
Oct 4, 2016

Conversation

nikomatsakis
Copy link
Contributor

The collector was asserting a total absence of projections, but some projections are expected, even in trans: in particular, projections containing higher-ranked regions, which we don't currently normalize.

r? @pnkfelix

Fixes #36381

assert!(concrete_substs.is_normalized_for_trans());
//assert!(concrete_substs.is_normalized_for_trans(),
// "concrete_substs not normalized for trans: {:?}",
// concrete_substs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only comment this out?

Copy link
Member

Choose a reason for hiding this comment

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

you mean versus remove it entirely?

Copy link
Member

Choose a reason for hiding this comment

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

Still, I have to admit: @nikomatsakis it does not seem like you are ever actually reading the newly added HAS_NORMALIZABLE_PROJECTION bit; you are merely setting it...? Did you intend to incorporate that bit into this assertion, and then you changed your mind and decided to just remove the assertion entirely?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this doesn't "loosen" the assertion, it disables it. Looks like the change to the assertion is incomplete or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Er, I didn't mean to commit that part.

@pnkfelix
Copy link
Member

pnkfelix commented Oct 3, 2016

@nikomatsakis r=me once you address the travis failure.

The collector was asserting a total absence of projections, but some
projections are expected, even in trans: in particular, projections
containing higher-ranked regions, which we don't currently normalize.
@nikomatsakis
Copy link
Contributor Author

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Oct 3, 2016

📌 Commit 58b75f7 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Oct 4, 2016

⌛ Testing commit 58b75f7 with merge 5ea241b...

bors added a commit that referenced this pull request Oct 4, 2016
loosen assertion against proj in collector

The collector was asserting a total absence of projections, but some projections are expected, even in trans: in particular, projections containing higher-ranked regions, which we don't currently normalize.

r? @pnkfelix

Fixes #36381
@bors bors merged commit 58b75f7 into rust-lang:master Oct 4, 2016
@nikomatsakis
Copy link
Contributor Author

@pnkfelix do you think we ought to beta backport, actually? This is a stable-to-stable regression, I guess.

@pnkfelix pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 10, 2016
@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 11, 2016
@nikomatsakis
Copy link
Contributor Author

Accepting for backport because tiny patch, fixes regression.

cc @rust-lang/compiler

@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 11, 2016
@nikomatsakis nikomatsakis deleted the issue-36381 branch April 14, 2017 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants