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

Make novel structural match violations not a bug #73446

Merged
merged 3 commits into from
Jun 19, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jun 17, 2020

Fixes (on master) #73431.

Ideally, CustomEq would emit a strict subset of the structural match errors that are found by search_for_structural_match_violation, since it allows more cases due to value-based reasoning. However, const qualification is more conservative than search_for_structural_match_violation around associated constants, since qualification does not try to substitute type parameters.

In the long term, we should probably make const qualification work for generic associated constants, but I don't like extending its capabilities even further.

r? @pnkfelix

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 17, 2020
@ecstatic-morse ecstatic-morse added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 17, 2020
@pnkfelix
Copy link
Member

Should the warning text reference some issue for further info?

@ecstatic-morse
Copy link
Contributor Author

Done. See #73448.

@pnkfelix
Copy link
Member

As discussed in private with @ecstatic-morse , i cannot immediately tell if this represents a hole in the structural-match check that we will need to plug (and thus maybe we should have something stronger than a warn! here, which will only fire if one uses RUSTC_LOG=warn or similar).

But it probably isn't worth the effort of putting in the plumbing to issue a proper lint warning here; instead effort should be focused on fixing #73448 itself.

and in any case, this PR as it stands fixes the ICE, which trumps all else at the moment.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 17, 2020

📌 Commit 3a1207f has been approved by pnkfelix

@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 Jun 17, 2020
@pnkfelix
Copy link
Member

unilaterally accepting for beta-backport.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 17, 2020
@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Jun 17, 2020

giving this priority since it is a backport accepted for backport
@bors p=1

tmandry added a commit to tmandry/rust that referenced this pull request Jun 17, 2020
Make novel structural match violations not a `bug`

Fixes (on master) rust-lang#73431.

Ideally, `CustomEq` would emit a strict subset of the structural match errors that are found by `search_for_structural_match_violation`, since it allows more cases due to value-based reasoning. However, const qualification is more conservative than `search_for_structural_match_violation` around associated constants, since qualification does not try to substitute type parameters.

In the long term, we should probably make const qualification work for generic associated constants, but I don't like extending its capabilities even further.

r? @pnkfelix
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 18, 2020
Make novel structural match violations not a `bug`

Fixes (on master) rust-lang#73431.

Ideally, `CustomEq` would emit a strict subset of the structural match errors that are found by `search_for_structural_match_violation`, since it allows more cases due to value-based reasoning. However, const qualification is more conservative than `search_for_structural_match_violation` around associated constants, since qualification does not try to substitute type parameters.

In the long term, we should probably make const qualification work for generic associated constants, but I don't like extending its capabilities even further.

r? @pnkfelix
@bors
Copy link
Contributor

bors commented Jun 18, 2020

⌛ Testing commit 3a1207f with merge 205c00429373b16ac70c645031eabc44fe4c8232...

@RalfJung
Copy link
Member

Yielding to rollup
@bors retry

@bors
Copy link
Contributor

bors commented Jun 18, 2020

⌛ Testing commit 3a1207f with merge 493d2a798de149fba00e07f2fb8f0f25f562b760...

@RalfJung
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Jun 18, 2020

⌛ Testing commit 3a1207f with merge b7a9cf51cc76a4992329c385a49862503e968c3e...

@bors
Copy link
Contributor

bors commented Jun 18, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 18, 2020
@Dylan-DPC-zz
Copy link

@bors retry p=5

@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 Jun 18, 2020
@bors
Copy link
Contributor

bors commented Jun 18, 2020

⌛ Testing commit 3a1207f with merge 036b5fe...

@bors
Copy link
Contributor

bors commented Jun 19, 2020

☀️ Test successful - checks-azure
Approved by: pnkfelix
Pushing 036b5fe to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 19, 2020
@bors bors merged commit 036b5fe into rust-lang:master Jun 19, 2020
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 26, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 27, 2020
…ulacrum

[beta] backports

This PR backports the following:

* Make novel structural match violations not a `bug` rust-lang#73446
* linker: Never pass `-no-pie` to non-gnu linkers rust-lang#73384
* Disable the `SimplifyArmIdentity` pass rust-lang#73262
* Allow inference regions when relating consts rust-lang#73225
* Fix link error with #[thread_local] introduced by rust-lang#71192 rust-lang#73065
* Ensure stack when building MIR for matches rust-lang#72941
* Don't create impl candidates when obligation contains errors rust-lang#73005

r? @ghost
@ecstatic-morse ecstatic-morse deleted the issue-73431 branch October 6, 2020 01:42
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
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. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants