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

Effective breakage to jiff due to ambiguous_negative_literals #128287

Open
traviscross opened this issue Jul 27, 2024 · 5 comments
Open

Effective breakage to jiff due to ambiguous_negative_literals #128287

traviscross opened this issue Jul 27, 2024 · 5 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@traviscross
Copy link
Contributor

We should discuss this bit of evidence that came in after the FCP completed on:

@BurntSushi said:

This completely broke Jiff once the change landed. Because it specifically supports negating spans like -1.hour(). In the case of Jiff, there is no ambiguity because -1.hour() and (-1).hour() and -(1.hour()) are all precisely equivalent.

cc @BurntSushi @rust-lang/lang

@rustbot labels +T-lang +I-lang-nominated

@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 27, 2024
@traviscross traviscross removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 27, 2024
@traviscross traviscross changed the title Breakage to jiff due to ambiguous_negative_literals Effective breakage to jiff due to ambiguous_negative_literals Jul 27, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Jul 27, 2024

This lint indeed assumes the operations are ordering-sensitive. Like abs. Perhaps the lint should be on the function, not the operator?

@traviscross traviscross added the C-discussion Category: Discussion or questions that doesn't represent real issues. label Jul 27, 2024
@Urgau
Copy link
Member

Urgau commented Jul 28, 2024

The partially-uplifted Clippy clippy::precedence lint (which made up this lint) had a basic heuristic for not linting in cases where the precedence of the operations didn't matter. It was part of the original PR, but @RalfJung preferred consistent linting [comment]:

Personally I'd prefer it to warn consistently, even if the semantics of the function mean it doesn't matter. (a) that seems more consistent and less surprising, and (b) I am writing tests which ensure that sin behaves correctly, and I care about which sign the argument actually has.

@Urgau
Copy link
Member

Urgau commented Jul 28, 2024

If it's decided that it's not worth linting on these cases, I would propose using the #[diagnostic] namespace by introducing an opt-out, that library authors could use on to suppress the lint for downstream users. Something like #[diagnostic::irrelevant_negative_literal_precedence] (better name to be found) which could applied to all the relevant function definitions.

@traviscross
Copy link
Contributor Author

We discussed this in our lang triage call today.

@Urgau: If you would, we'd like this lint changed to allow-by-default while we consider this question further.

cc @compiler-errors

tgross35 added a commit to tgross35/rust that referenced this issue Jul 31, 2024
… r=compiler-errors

Temporarily switch `ambiguous_negative_literals` lint to allow

This PR temporarily switch the `ambiguous_negative_literals` lint to `allow-by-default`, as asked by T-lang in rust-lang#128287 (comment).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 31, 2024
… r=compiler-errors

Temporarily switch `ambiguous_negative_literals` lint to allow

This PR temporarily switch the `ambiguous_negative_literals` lint to `allow-by-default`, as asked by T-lang in rust-lang#128287 (comment).
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 1, 2024
Rollup merge of rust-lang#128449 - Urgau:tmp-allow-negative-lit-lint, r=compiler-errors

Temporarily switch `ambiguous_negative_literals` lint to allow

This PR temporarily switch the `ambiguous_negative_literals` lint to `allow-by-default`, as asked by T-lang in rust-lang#128287 (comment).
@traviscross traviscross added I-lang-nominated Nominated for discussion during a lang team meeting. and removed I-lang-nominated Nominated for discussion during a lang team meeting. labels Aug 1, 2024
@Urgau
Copy link
Member

Urgau commented Aug 1, 2024

As asked the lint was changed to allow-by-default in #128449.

BurntSushi added a commit to BurntSushi/jiff that referenced this issue Aug 15, 2024
In favor of a global one. But also, while these aren't technically
needed any more[1,2], we leave them be for now while things settle.

[1]: rust-lang/rust#128449
[2]: rust-lang/rust#128287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants