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

Overhaul rustc_middle::limits #136671

Merged
merged 5 commits into from
Feb 17, 2025
Merged

Overhaul rustc_middle::limits #136671

merged 5 commits into from
Feb 17, 2025

Conversation

nnethercote
Copy link
Contributor

In particular, to make pattern_complexity work more like other limits, which then enables some other simplifications.

r? @Nadrieril

@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 Feb 7, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 7, 2025

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred in exhaustiveness checking

cc @Nadrieril

@nnethercote
Copy link
Contributor Author

The last commit renames the unstable pattern_complexity attribute; I'm not sure if renaming such attributes is allowed.

cc @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

Sign in to view
Veykril
Veykril reviewed Feb 7, 2025
View reviewed changes
Copy link
Member

@Veykril Veykril left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You can't use rustc_session in rust-analyzer (and thus not in the relevant pattern matching api)

@GuillaumeGomez
Copy link
Member

I don't mind the renaming, but it sounds weird "allow pattern_complexity_limits". It's supposed to make sense in a sentence. 😉

@nnethercote
Copy link
Contributor Author

You can't use rustc_session in rust-analyzer (and thus not in the relevant pattern matching api)

Oh. Well, those changes are easy enough to remove, I can just use usize directly instead of importing Limit.

In what sense can't it be used? It seemed to compile ok, and a few other rustc crates are seemingly used, like rustc_index, rustc_abi, rustc_pattern_analysis. Is this a technical thing, a policy thing, something else?

@Veykril
Copy link
Member

Veykril commented Feb 7, 2025

rust-analyzer needs to build on stable (as well as not depend on specifically rustc_span due to the global interning) which puts a bunch of limits onto the compiler crates it depends on (most have feature cfgs to make this work), rustc_session is not one of those (and is for example a gated dependency of rustc_pattern_analysis)

@nnethercote
Copy link
Contributor Author

I don't mind the renaming, but it sounds weird "allow pattern_complexity_limits". It's supposed to make sense in a sentence. 😉

It's a crate-level attribute with a parameter, so you use it like #![pattern_complexity_limit = "1000"], just like recursion_limit. That seems better than the current #![pattern_complexity = "1000"].

@GuillaumeGomez
Copy link
Member

Oh I see, makes more sense now. :)

@nnethercote
Copy link
Contributor Author

Thanks for the comments and explanations. I have changed things so that rust-analyzer doesn't depend on rustc_session and CI tests are passing, and Guillaume seems happy. @Nadrieril, I think this is ready for review, thanks.

@bors
Copy link
Contributor

bors commented Feb 13, 2025

☔ The latest upstream changes (presumably #136943) made this pull request unmergeable. Please resolve the merge conflicts.

Nadrieril
Nadrieril requested changes Feb 16, 2025
View reviewed changes
Copy link
Member

@Nadrieril Nadrieril left a comment

Choose a reason for hiding this comment

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

Nice, I didn't know there was a shared infrastructure for limits, this is better like this. r=me once the pattern_analysis tests can also run on stable.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2025
@nnethercote
Add pattern_complexity_limit to Limits.
b023671 
It's similar to the other limits, e.g. obtained via `get_limit`. So it
makes sense to handle it consistently with the other limits. We now use
`Limit`/`usize` in most places instead of `Option<usize>`, so we use
`Limit::new(usize::MAX)`/`usize::MAX` to emulate how `None` used to work.

The commit also adds `Limit::unlimited`.
@nnethercote
Merge get_limit and get_limit_size.
30b8c84 
Thanks to the previous commit, they no longer need to be separate.
@nnethercote
Improve comments about limits.
13280ee
@nnethercote
Move rustc_middle::limits to rustc_interface.
223c95f 
It's always good to make `rustc_middle` smaller. `rustc_interface` is
the best destination, because it's the only crate that calls
`get_recursive_limit`.
@nnethercote
Rename pattern_complexity attr as pattern_complexity_limit.
7a8c0fc 
For consistency with `recursion_limit`, `move_size_limit`, and
`type_length_limit`.
@nnethercote
Copy link
Contributor Author

I addressed the comments.

@bors r=Nadrieril rollup

@bors
Copy link
Contributor

bors commented Feb 16, 2025

📌 Commit 7a8c0fc 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 16, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 16, 2025
@workingjubilee
Rollup merge of rust-lang#136671 - nnethercote:middle-limits, r=Nadri…
5603b6f 
…eril

Overhaul `rustc_middle::limits`

In particular, to make `pattern_complexity` work more like other limits, which then enables some other simplifications.

r? `@Nadrieril`
@workingjubilee workingjubilee mentioned this pull request Feb 16, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
@bors
Auto merge of rust-lang#137153 - workingjubilee:rollup-1cjmnuh, r=wor…
57fb317 
…kingjubilee

Rollup of 12 pull requests

Successful merges:

 - rust-lang#136671 (Overhaul `rustc_middle::limits`)
 - rust-lang#136817 (Pattern Migration 2024: clean up and comment)
 - rust-lang#136844 (Use `const_error!` when possible)
 - rust-lang#136953 (rustc_target: import TargetMetadata)
 - rust-lang#137095 (Replace some u64 hashes with Hash64)
 - rust-lang#137100 (HIR analysis: Remove unnecessary abstraction over list of clauses)
 - rust-lang#137105 (Restrict DerefPure for Cow<T> impl to T = impl Clone, [impl Clone], str.)
 - rust-lang#137120 (Enable `relative-path-include-bytes-132203` rustdoc-ui test on Windows)
 - rust-lang#137125 (Re-add missing empty lines in the releases notes)
 - rust-lang#137140 (Fix const items not being allowed to be called `r#move` or `r#static`)
 - rust-lang#137145 (use add-core-stubs / minicore for a few more tests)
 - rust-lang#137149 (Remove SSE ABI from i586-pc-windows-msvc)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 17, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
@bors
Auto merge of rust-lang#137163 - matthiaskrgr:rollup-ovgfkns, r=matth…
273465e 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#136466 (Start removing `rustc_middle::hir::map::Map`)
 - rust-lang#136671 (Overhaul `rustc_middle::limits`)
 - rust-lang#136817 (Pattern Migration 2024: clean up and comment)
 - rust-lang#136844 (Use `const_error!` when possible)
 - rust-lang#137080 (bootstrap: add more tracing to compiler/std/llvm flows)
 - rust-lang#137101 (`invalid_from_utf8[_unchecked]`: also lint inherent methods)
 - rust-lang#137140 (Fix const items not being allowed to be called `r#move` or `r#static`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0c051c8 into rust-lang:master Feb 17, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 17, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
@rust-timer
Unrolled build for rust-lang#136671
7159f0b 
Rollup merge of rust-lang#136671 - nnethercote:middle-limits, r=Nadrieril

Overhaul `rustc_middle::limits`

In particular, to make `pattern_complexity` work more like other limits, which then enables some other simplifications.

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

@Veykril Veykril Veykril left review comments

@Nadrieril Nadrieril Nadrieril requested changes

Assignees

@Nadrieril Nadrieril

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
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @rustbot @rust-log-analyzer @GuillaumeGomez @Veykril @bors @Nadrieril