Implement "unused pattern bindings" p2022 impl #6460
Conversation
This adds an "unused" keyword and various diagnostics related to it. The implementation is slightly more general than necessary and prepares the ground for other diagnostics that can be implemented using dataflow analysis on function bodies (such as flow-checking).
burakemir
force-pushed
the
unused_pattern_bindings_p2022_impl
branch
from
be01921 to
33de1d9
Compare
|
One detail: when a variable is only used to access some type, and not used otherwise, it still counts as unused. See toolchain/check/testdata/class/nested_name.carbon I feel it may be better to keep detection of unusedness in types for a different PR. |
danakj
left a comment
danakj
left a comment
There was a problem hiding this comment.
I have only reviewed the parse stuff so far.
Can we move the check/ changes into another PR, so that we can review them in isolation? https://github.com/carbon-language/carbon-lang/pull/6255/files#diff-c7f2de42def7e3e0072ee66501047e5e72e0578acae95f5e6a4037b1b4648b2b is an example of another PR that does similar.
Then we'd also have the option of letting check just ignore unused and we could insert it into tests as a PR on its own, then follow that with the changes to check/ to enforce it, along with any test changes that result from enforcement specifically?
| // TIP: To dump output, run: | ||
| // TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/parse/testdata/var/fail_unused.carbon | ||
|
|
||
| var unused x: i32; |
There was a problem hiding this comment.
Is this supposed to have an error?
| // CHECK:STDERR: ^~~ | ||
| // CHECK:STDERR: | ||
| var ref unused y: i32; | ||
| var unused _: i32; |
There was a problem hiding this comment.
Is this supposed to have an error?
We want to avoid mixing failures and passes in the same file split, so that if one flips to the other, we get a FAIL from the overall test run.
This set of tests here seems both a little oddly specific but also beyond the scope of unused so I am not sure what to make of it.
It's not testing putting unused in all possible positions, but it is testing putting var and ref together. So maybe we need more tests here, to verify parsing unused in all the positions.
I would suggest using function parameters, cuz you can't even write ref on a variable decl.
| @@ -156,4 +156,23 @@ auto HandleFinishVariablePattern(Context& context) -> void { | |||
| } | |||
| } | |||
|
|
|||
| auto HandleUnusedPattern(Context& context) -> void { | |||
There was a problem hiding this comment.
Should these go in a handle_unused.cpp? They don't seem specific to var at all
| // ^~~~~~ | ||
| // 1. Pattern | ||
| // 2. FinishUnusedPattern | ||
| CARBON_PARSE_STATE(UnusedPattern) |
There was a problem hiding this comment.
Nothing above currently references UnusedPattern which doesn't look right. See for example on L1033 where VariablePattern is referenced. We'll need to explain/enumerate all the places UnusedPattern can show up.
Also in #6448 (comment), @zygoloid referred to unused as a "pattern operator" and maybe it would make sense to use that name here as well, rather than calling it a Pattern which sounds incorrect to me?
It probably makes sense to move these up beside Pattern rather than down here - this file is not alphabetical.
2 participants
This implements proposal #2022, with changes from #3763 and leads answers on #6448
Detection of unusedness is happening in
toolchain/check/dataflow_analysis.cppAll test cases with unused bindings were updated in order to avoid polluting test output.