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

Always force non-trimming of path in unreachable_patterns lint #135310

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jan 9, 2025

Creating a "trimmed DefID path" when no error is being emitted is an ICE (on purpose). If we create a trimmed path for a lint that is then silenced before being emitted causes a known ICE. This side-steps the issue by always using with_no_trimmed_path!.

This was verified to fix https://github.com/quinn-rs/quinn/, but couldn't write a repro case for the test suite.

Fix #135289.

Creating a "trimmed DefID path" when no error is being emitted is an ICE (on purpose). If we create a trimmed path for a lint that is then silenced before being emitted causes a known ICE. This side-steps the issue by always using `with_no_trimmed_path!`.

This was verified to fix https://github.com/quinn-rs/quinn/, but couldn't write a repro case for the test suite.

Fix rust-lang#135289.
@rustbot
Copy link
Collaborator

rustbot commented Jan 9, 2025

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 9, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 9, 2025

Some changes occurred in match checking

cc @Nadrieril

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
curl: (22) The requested URL returned error: 404
curl: (22) The requested URL returned error: 404
ERROR: failed to download llvm from ci

    HELP: There could be two reasons behind this:
        1) The host triple is not supported for `download-ci-llvm`.
        2) Old builds get deleted after a certain time.
    HELP: In either case, disable `download-ci-llvm` in your config.toml:
    [llvm]
    download-ci-llvm = false
    
Build completed unsuccessfully in 0:00:22

@lqd
Copy link
Member

lqd commented Jan 9, 2025

I had the same workaround locally, but still wondered where the lint was silenced for the unreachable arms in that crate 🤔.

@estebank
Copy link
Contributor Author

estebank commented Jan 9, 2025

@lqd my assumption it is because of cargo silencing the lint for deps, but it's only a guess.

@lqd
Copy link
Member

lqd commented Jan 10, 2025

Interesting possibility. They do that via --cap-lints=allow IIRC, so I would have expected this to allow unreachable patterns, that you check at the beginning of the typo const checking. But maybe they do so differently for this use-case via tests.

@compiler-errors
Copy link
Member

Would be nice to have a test for this

@Nadrieril
Copy link
Member

I don't get why a lint wasn't emitted, I too would really like a repro.

@estebank
Copy link
Contributor Author

estebank commented Jan 10, 2025

I identified it is happening at quinn-proto/src/tests/mod.rs:1640:9 and now I think it might be cross-crate macro shenanigans affecting the logic of lint emitting. I'll dig deeper.

Edit: If I don't find a more appropriate solution before then, we should beta-backport this change in 4/5 weeks.

@compiler-errors
Copy link
Member

#135673 (comment)

@rustbot label: +beta-nominated

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 18, 2025
@Nadrieril
Copy link
Member

well, merging because ICE and backports, would be nice to get a repro eventually but also shrug

@bors r+

@bors
Copy link
Contributor

bors commented Jan 19, 2025

📌 Commit 93a1950 has been approved by Nadrieril

It is now in the queue for this repository.

@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 Jan 19, 2025
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 19, 2025
Always force non-trimming of path in `unreachable_patterns` lint

Creating a "trimmed DefID path" when no error is being emitted is an ICE (on purpose). If we create a trimmed path for a lint that is then silenced before being emitted causes a known ICE. This side-steps the issue by always using `with_no_trimmed_path!`.

This was verified to fix https://github.com/quinn-rs/quinn/, but couldn't write a repro case for the test suite.

Fix rust-lang#135289.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2025
…llaumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#135310 (Always force non-trimming of path in `unreachable_patterns` lint)
 - rust-lang#135446 (further improve panic_immediate_abort by removing rtprintpanic! messages)
 - rust-lang#135491 (Remove dead rustc_allowed_through_unstable_modules for std::os::fd contents)
 - rust-lang#135542 (Add the concrete syntax for precise capturing to 1.82 release notes.)
 - rust-lang#135700 (Emit single privacy error for struct literal with multiple private fields and add test for `default_field_values` privacy)
 - rust-lang#135729 (Add debug assertions to compiler profile)
 - rust-lang#135736 (rustdoc: Fix flaky doctest test)
 - rust-lang#135738 (Replace usages of `map_or(bool, ...)` with `is_{some_and|none_or|ok_and}`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2025
Rollup of 5 pull requests

Successful merges:

 - rust-lang#134276 (fully de-stabilize all custom inner attributes)
 - rust-lang#135237 (Match Ergonomics 2024: document and reorganize the currently-implemented feature gates)
 - rust-lang#135310 (Always force non-trimming of path in `unreachable_patterns` lint)
 - rust-lang#135446 (further improve panic_immediate_abort by removing rtprintpanic! messages)
 - rust-lang#135491 (Remove dead rustc_allowed_through_unstable_modules for std::os::fd contents)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3f2f695 into rust-lang:master Jan 20, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 20, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2025
Rollup merge of rust-lang#135310 - estebank:issue-135289, r=Nadrieril

Always force non-trimming of path in `unreachable_patterns` lint

Creating a "trimmed DefID path" when no error is being emitted is an ICE (on purpose). If we create a trimmed path for a lint that is then silenced before being emitted causes a known ICE. This side-steps the issue by always using `with_no_trimmed_path!`.

This was verified to fix https://github.com/quinn-rs/quinn/, but couldn't write a repro case for the test suite.

Fix rust-lang#135289.
@estebank
Copy link
Contributor Author

@Nadrieril #135289 (comment)

@Nadrieril
Copy link
Member

thx for your investigation @estebank btw!

@lqd
Copy link
Member

lqd commented Jan 20, 2025

A fixme would also be good, at least to track that the fix is suboptimal and more of a workaround for the ICE. But the test is a prerequisite for analysis to even know what the principled fix is in the first place. Maybe others can help.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 21, 2025
Add fixme and test for issue rust-lang#135289

This PR:
- adds a test minimizing issue rust-lang#135289 for PR rust-lang#135310
- adds a fixme about the suboptimal fix for the ICE

I've verified the test indeed ICEs with 3f2f695 reverted.

r? `@estebank`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2025
Rollup merge of rust-lang#135833 - lqd:add-ice-test, r=compiler-errors

Add fixme and test for issue rust-lang#135289

This PR:
- adds a test minimizing issue rust-lang#135289 for PR rust-lang#135310
- adds a fixme about the suboptimal fix for the ICE

I've verified the test indeed ICEs with 3f2f695 reverted.

r? `@estebank`
@apiraino
Copy link
Contributor

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels handled by them.

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 23, 2025
@cuviper cuviper mentioned this pull request Jan 24, 2025
@cuviper cuviper modified the milestones: 1.86.0, 1.85.0 Jan 24, 2025
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 24, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2025
[beta] backports

- Always force non-trimming of path in `unreachable_patterns` lint rust-lang#135310
- Add Profile Override for Non-Git Sources rust-lang#135433
- resolve symlinks of LLVM tool binaries before copying them rust-lang#135585
- add cache to `AmbiguityCausesVisitor` rust-lang#135618
- When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely rust-lang#135643
- Temporarily bring back `Rvalue::Len` rust-lang#135709
- make it possible to use ci-rustc on tarball sources rust-lang#135722
- Remove test panic from File::open rust-lang#135837
- Only assert the `Parser` size on specific arches rust-lang#135855

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2025
[beta] backports

- Always force non-trimming of path in `unreachable_patterns` lint rust-lang#135310
- Add Profile Override for Non-Git Sources rust-lang#135433
- resolve symlinks of LLVM tool binaries before copying them rust-lang#135585
- add cache to `AmbiguityCausesVisitor` rust-lang#135618
- When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely rust-lang#135643
- Temporarily bring back `Rvalue::Len` rust-lang#135709
- make it possible to use ci-rustc on tarball sources rust-lang#135722
- Remove test panic from File::open rust-lang#135837
- Only assert the `Parser` size on specific arches rust-lang#135855
- [beta] TRPL: more backward-compatible Edition changes rust-lang#135843

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 26, 2025
[beta] backports

- Always force non-trimming of path in `unreachable_patterns` lint rust-lang#135310
- Add Profile Override for Non-Git Sources rust-lang#135433
- resolve symlinks of LLVM tool binaries before copying them rust-lang#135585
- add cache to `AmbiguityCausesVisitor` rust-lang#135618
- When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely rust-lang#135643
- make it possible to use ci-rustc on tarball sources rust-lang#135722
- Remove test panic from File::open rust-lang#135837
- Only assert the `Parser` size on specific arches rust-lang#135855
- [beta] TRPL: more backward-compatible Edition changes rust-lang#135843

r? cuviper
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. 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.

ICE: trimmed_def_paths called, diagnostics were expected but none were emitted
9 participants