Skip to content

tests/ui/binop: add annotations for reference rules#153549

Open
DanielEScherzer wants to merge 1 commit intorust-lang:mainfrom
DanielEScherzer:test-references-binop
Open

tests/ui/binop: add annotations for reference rules#153549
DanielEScherzer wants to merge 1 commit intorust-lang:mainfrom
DanielEScherzer:test-references-binop

Conversation

@DanielEScherzer
Copy link
Contributor

r? ehuss
@rustbot label +A-docs

@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. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools labels Mar 7, 2026
@@ -1,4 +1,5 @@
//@ run-pass
//@ reference: patterns.literal.intro
Copy link
Contributor

Choose a reason for hiding this comment

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

This test looks like it is checking the parsing of 1-1 and negative literals in patterns. Perhaps also add patterns.literal.syntax and expr.arith-logic.syntax?

(In the long-term, I'd like to avoid using .intro rules since they are supposed to be introductions, and not specifications of behavior. But we're not there, yet.)

Copy link
Contributor Author

@DanielEScherzer DanielEScherzer Mar 9, 2026

Choose a reason for hiding this comment

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

Added patterns.literal.syntax, not sure about expr.arith-logic.syntax since this is mostly about the negative pattern and not the arithmetic syntax

about .intro rules - makes sense, but as long as they exist and are included in the test coverage summary I figured covering them would be useful

Copy link
Contributor

@ehuss ehuss Mar 10, 2026

Choose a reason for hiding this comment

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

Looking at the history of the file, the assert_eq!(1-1, 0) seems to be specifically saying does 1-1 work? Specifically #954 and f26e770 (which ultimately culminated in 7655b3c which removed lexing of negative literals).

There I believe they were dealing with the parsing of expr-expr where the parser was incorrectly handling negative literals as a 1 followed by a -1 literal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think to check the history, added expr.arith-logic.syntax too

@@ -1,4 +1,5 @@
//@ run-pass
//@ reference: expr.negate.results
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this test is covering all operator overloading in general. I would expect to also include expr.arith-logic.behavior and expr.array.index.trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For expr.arith-logic.behavior I didn't include that because it doesn't cover all of the rule cases, and I figured there is probably a better test to point to in another directory

similarly for expr.array.index.trait because this doesn't showcase a mutable reference I thought there would be a better test elsewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, this is part of the difficult of connecting the reference rules to the rustc test suite. I think the majority of the tests are not going to be a 100% coverage or connection to the rule.

We've talked a bunch about how to handle this, but have not settled on a better solution. For now I think we should just try to associate each test with whatever rule it is intending to cover, even if it isn't complete.

In the long term, we'll probably want to have some kind of hybrid approach where we write our own tests that very clearly and directly cover a reference rule. But we haven't settled on exactly how to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Writing our own tests for 100% coverage would make sense - e.g. I didn't add to https://github.com/rust-lang/rust/blob/2d76d9bc76f27b03b4899e72ce561c7ac2c5cf6b/tests/ui/binop/binops.rs because it didn't fully test any of the rules. I figured that once I had reviewed a majority of the ui directories I would maybe write tests for things that were not explicitly tested

@DanielEScherzer DanielEScherzer force-pushed the test-references-binop branch from ff77acd to 70db796 Compare March 9, 2026 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants