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

Avoid invalid NaN lint machine-applicable suggestion in const context #114486

Merged
merged 2 commits into from Aug 6, 2023

Conversation

Urgau
Copy link
Contributor

@Urgau Urgau commented Aug 4, 2023

This PR removes the machine-applicable suggestion in const context for the invalid_nan_comparision lint and replace it with a simple help.

Fixes #114471

@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2023

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

@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 Aug 4, 2023
@compiler-errors
Copy link
Member

I wonder if we should just see what's blocking stabilizing is_nan in const contexts 😆

@Urgau
Copy link
Contributor Author

Urgau commented Aug 5, 2023

I wonder if we should just see what's blocking stabilizing is_nan in const contexts laughing

The tracking issue #72505, say that it's blocked on #57241, ie general availability of floating point types in const fn.

@compiler-errors
Copy link
Member

Well in that case, shouldn't we just be suggesting nothing? I don't think turning this into a help that still tells folks to use the same fn is the right solution.

@Urgau Urgau force-pushed the const-context-nan-suggestion-114471 branch from cc264a1 to b71f2be Compare August 5, 2023 21:55
@Urgau
Copy link
Contributor Author

Urgau commented Aug 5, 2023

It's a bit unsatisfying to not have any suggestion for const context, but I agree, better suggest nothing than suggest functions that have no plan to stabilization in the short or medium term.

Updated the PR to completely remove the suggestion.

e: &hir::Expr<'_>,
l: &hir::Expr<'_>,
r: &hir::Expr<'_>,
f: impl FnOnce(Span, Span) -> InvalidNanComparisonsSuggestion,
) -> InvalidNanComparisons {
let suggestion =
let suggestion = (!cx.tcx.hir().is_inside_const_context(e.hir_id)).then(|| {
Copy link
Member

Choose a reason for hiding this comment

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

nit: this is a somewhat confusing way of avoiding an if statement 😅

compiler/rustc_lint/src/types.rs Show resolved Hide resolved
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 6, 2023

📌 Commit 3b3e466 has been approved by compiler-errors

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 Aug 6, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 6, 2023
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#114486 (Avoid invalid NaN lint machine-applicable suggestion in const context)
 - rust-lang#114503 (Remove invalid lint when there is a generic argument in prefix path)
 - rust-lang#114509 (Migrate GUI colors test to original CSS color format)
 - rust-lang#114524 (Also ICE when goals go from Ok to Err in new solver)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1305a43 into rust-lang:master Aug 6, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 6, 2023
@Urgau Urgau deleted the const-context-nan-suggestion-114471 branch August 6, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

invalid_nan_comparisons suggests invalid 5f32.is_nan() in const context
5 participants