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

Emit a warning if a match is too complex #122685

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 18, 2024

Based on the results of crater run from #121979, we can safely set this limit without introducing regressions in the current rust ecosystem.

Follow-up of #121917.

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 Mar 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2024

Some changes occurred in exhaustiveness checking

cc @Nadrieril

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2024

Some changes occurred in match checking

cc @Nadrieril

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Comment on lines 409 to 411
// Default value to emit the warning for "too complex" match. We picked it based on
// this crater run: <https://github.com/rust-lang/rust/pull/121979>.
.or(Some(10_000_000));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Default value to emit the warning for "too complex" match. We picked it based on
// this crater run: <https://github.com/rust-lang/rust/pull/121979>.
.or(Some(10_000_000));
// Default value to emit the warning for "too complex" match. We picked it to warn after a second or two.
.or(Some(10_000_000));

I don't think the crater matters much to future readers of this code, but the timing intent does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually you said that limit took more like 20s on your machine? I'd be curious to crater a lower limit then, because I'm afraid people won't wait 20s

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine by me!

@Nadrieril
Copy link
Contributor

Beautiful :D One last remark

@Nadrieril
Copy link
Contributor

Dear T-lang, this PR adds a warning that cannot be silenced, triggered when a match takes a really long time to analyze (in the order of seconds). This is to help users figure out what's taking so long and fix it.

We could make the limit configurable or the warning allowable. I argue that's not necessary because crater showed zero regressions with the current limit, and it's be pretty easy in general to split up a match into smaller matches to avoid blowup.

We're still figuring out the exact limit, but does the team approve in principle?

@Nadrieril Nadrieril added the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Mar 18, 2024
@GuillaumeGomez
Copy link
Member Author

Updated!

@GuillaumeGomez
Copy link
Member Author

Fixed typo.

@Nadrieril Nadrieril added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Mar 25, 2024
@traviscross
Copy link
Contributor

traviscross commented Apr 8, 2024

Those interested in pushing the limits of exhaustiveness checking may be interested in:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). 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.

None yet

7 participants