Implement unused pattern bindings, continued by burakemir · Pull Request #6518 · carbon-language/carbon-lang

burakemir · 2025-12-17T19:53:39Z

Implementation of unused pattern bindings #2022, continued.

Whereas previous PR #6460 took care of parsing, and PR #6479 prepared the stage by using _ in some test cases, this PR has the the actual implementation, using a simple dataflow analysis.

Implements the following diagnostics in check: * [UnusedBinding] warning, a binding is unused. * [UnusedPatternNoBindings] warning, "unused" marker has no effect * [UnusedMarkerInDeclaration] error, "unused" is for definitions * [UnusedButUsed] error, marker is contradicting use * [UnusedButUsedHere] note, shows first use Updates all test cases which contain unused markers, and replaces TODOs with actual warnings (or evidence of absence of warnings). Unfortunately, the prevalence of unused bindings in test cases makes the change rather large.

jonmeow

Thanks for the contribution! I've done a first pass.

In particular regarding my comments on CARBON_KIND_SWITCH and things like AnyBindingPattern, note that comes up multiple times in this PR. We use those a lot to reduce the weight of these sorts of repeat inst kind checks, hopefully they're helpful here!

toolchain/check/dataflow_analysis.cpp

toolchain/sem_ir/entity_name.h

toolchain/diagnostics/diagnostic_kind.def

toolchain/check/pattern.h

toolchain/check/dataflow_analysis.cpp

toolchain/check/testdata/dataflow/unused.carbon

burakemir

Thanks for the suggestions! PTAL.

toolchain/check/dataflow_analysis.cpp

toolchain/check/handle_function.cpp

toolchain/check/handle_binding_pattern.cpp

toolchain/check/dataflow_analysis.cpp

jonmeow

Stepping back a moment, I was thinking again about the responses on #6448. I went back to @zygoloid because of this comment, trying to figure out when he meant that changing a name to _ should diagnose. After discussion, here are a couple specific things:

fn F(x: i32) {
  return;
  // When a non-`unused` binding is used in unreachable code,
  // that should not diagnose because changing it to `_` would be a name lookup diagnostic here.
  x = 0;
}

fn F(unused x: i32) {
  return;
  // When an explicitly `unused` name is used in unreachable code,
  // both warning and not warning are okay choices.
  x = 0;
}

I was looking through tests, trying to figure out if cases like this are covered (it looks like tests may not be covering unreachable dataflow?). In particular I'm noting this, because I think it may have interesting implications on dataflow; is the current setup giving desired answer for references to x: i32 (no unused)?

For return flow, zygoloid reminded me we have IsCurrentPositionReachable. We use this to take advantage of some flow data that check accumulates (rather than as a separate dataflow step, as proposed here). Had you looked at using IsCurrentPositionReachable for the unused-but-used case?

toolchain/check/handle_function.cpp

toolchain/check/dataflow_analysis.cpp

toolchain/check/handle_function.cpp

toolchain/check/dataflow_analysis.cpp

toolchain/check/merge.cpp

jonmeow · 2025-12-18T20:58:55Z

Also, quick small note in addition to the above, I know it's a big question but I'm also going to be out soon for a while; I'll probably be handing off review tomorrow since even though people are out a lot for the holidays, I'm probably coming back later than others. :)

jonmeow

Thanks! I think this is close to being ready to submit. Can you please update it against trunk, when you think it's ready for a merge? That way we can handle actually doing the merge.

toolchain/check/scope_stack.h

toolchain/check/handle_binding_pattern.cpp

burakemir

Thanks for the review, Will rebase now.

toolchain/check/scope_stack.h

toolchain/check/handle_binding_pattern.cpp

burakemir · 2026-02-13T13:28:38Z

Added a merge commit. This should be good to land. Note that I don't have rights to merge.

jonmeow · 2026-02-13T17:40:27Z

It looks like you still have conflicts, with older PRs (e.g. toolchain/check/BUILD last updated a week ago). Would you be interested in fixing the outstanding conflicts, or do you want me to finish the merge work?

To be sure, you should probably be doing something like git fetch upstream && git merge upstream/trunk to fetch the latest changes. You can also double-check on the PR to look if GitHub says there are still outstanding conflicts.

…ngs_impl_continued

burakemir · 2026-02-16T13:31:29Z

Done the proper merge commit from trunk now.
Please merge.

jonmeow

It looks like a PR got in. I was getting to merge that (you might note me having pushed to this PR, I undid it), but was double-checking tests were passing (bazel test //...) before that. It looks like you have a couple failing tests already, so I backed out my merge -- do you want to fix tests and merge again?

jonmeow · 2026-02-17T20:56:08Z

toolchain/check/testdata/interface/generic_default_fn.carbon

+// CHECK:STDERR: generic_default_fn.carbon:[[@LINE+4]]:6: warning: binding `T` unused [UnusedBinding]
+// CHECK:STDERR: fn I(T:! type).F[ref self: Self]() -> Self* { return &self; }
+// CHECK:STDERR:      ^
+// CHECK:STDERR:


This test is failing. Maybe this could be _:! type?

We cannot use _ here, since proposal #3763 explicitly disallows this and considers it a difference in redeclaration.

However, if we were to try "fn I(unused T:! type).F ..." we would find that the toolchain's generic-handling code does not handle unused correctly in either construction or comparison of eval blocks (or both). This is explicitly left as TODO, and I would prefer to not expand the scope of this PR further.

To be very clear: the test is not failing for me, with the warning being present in the expected output. I propose to just leave that warning in.

jonmeow · 2026-02-17T20:57:10Z

toolchain/check/testdata/impl/incomplete.carbon

+// CHECK:STDERR: nested_require_incomplete_interface.carbon:[[@LINE+4]]:6: warning: binding `T` unused [UnusedBinding]
+// CHECK:STDERR: fn F(T:! X where .X1 = {}) {}
+// CHECK:STDERR:      ^
+// CHECK:STDERR:


Can you add an unused marker here?

…ngs_impl_continued Add "unused" to new tests.

burakemir · 2026-02-18T22:10:39Z

Thanks for taking a look. It is not the case that I had a couple failing tests (wouldn't presubmit have caught this?), it is rather that everytime a test case gets added anywhere, it likely contains unused variables which are now diagnosed. This is one reason why I had to catch up the 150 commits with a strategy of rebase-in-stages before doing the merge.

jonmeow · 2026-02-19T20:49:38Z

Thanks for all your work here, I'll finish and resolve any remaining merge issues.

wouldn't presubmit have caught this?

Unfortunately conflicts prevent GitHub actions from running; the local pre-commit tooling doesn't run tests because we value it being fast.

burakemir added 2 commits December 17, 2025 18:52

Fix tests after merge

a5199c2

burakemir requested a review from a team as a code owner December 17, 2025 19:53

burakemir requested review from jonmeow and removed request for a team December 17, 2025 19:53

github-actions bot added the toolchain label Dec 17, 2025

Fix typo in diagnostic message

5bd6ae4

danakj changed the title ~~Implemend unused pattern bindings, continued~~ Implement unused pattern bindings, continued Dec 17, 2025

Add another unused marker

6fffd72

jonmeow reviewed Dec 17, 2025

View reviewed changes

burakemir added 5 commits December 18, 2025 13:48

Apply a round of reviewer comments

31627b6

Apply reviewer comments in handle_binding_pattern and handle_function

facb3b5

Applied reviewer comments

98cdb48

Add comments and move macro call

13c1136

Make Fact a template

db8d422

burakemir commented Dec 18, 2025

View reviewed changes

burakemir requested a review from jonmeow December 18, 2025 15:52

jonmeow reviewed Dec 18, 2025

View reviewed changes

burakemir added 11 commits December 19, 2025 10:47

Add test cases for unreachable code

638e127

apply reviewer comments

ef40178

make non-recursive

6bd96df

Use ID instead of int32_t for ref_params

db389be

Use ID instead of int32_t for first_use

4735674

replace VarInfo with pair

7c353f5

replace Fact with pair

529c6e3

Use AnyBinding instead of switch

8f69d79

remove leftover TODOs

24dfa6a

rewrite into one switch and AnyBinding/Param

c6f8fc7

Remove recursion, add links to free resources

23ee44f

burakemir added 2 commits February 6, 2026 17:18

Fix comment.

b58716e

Remove recursion, following project coding style rules.

8046a92

burakemir requested a review from jonmeow February 9, 2026 12:12

jonmeow reviewed Feb 9, 2026

View reviewed changes

toolchain/check/scope_stack.h Outdated Show resolved Hide resolved

toolchain/check/handle_binding_pattern.cpp Outdated Show resolved Hide resolved

Remove returned_var_name_id

51a053a

burakemir commented Feb 10, 2026

View reviewed changes

toolchain/check/scope_stack.h Outdated Show resolved Hide resolved

toolchain/check/handle_binding_pattern.cpp Outdated Show resolved Hide resolved

burakemir and others added 2 commits February 10, 2026 07:58

Remove size argument

b607472

Merge commit, ready to land.

d24cd3e

burakemir requested a review from a team as a code owner February 13, 2026 13:26

burakemir requested a review from jonmeow February 13, 2026 13:27

zygoloid removed their request for review February 13, 2026 21:14

Merge remote-tracking branch 'origin/trunk' into unused_pattern_bindi…

00d166d

…ngs_impl_continued

jonmeow force-pushed the unused_pattern_bindings_impl_continued branch from 36ce922 to 00d166d Compare February 17, 2026 21:11

jonmeow reviewed Feb 17, 2026

View reviewed changes

Burak Emir added 2 commits February 18, 2026 18:09

Merge remote-tracking branch 'origin/trunk' into unused_pattern_bindi…

14b2507

…ngs_impl_continued Add "unused" to new tests.

Add unused modified to incomplete.carbon

046fed9

jonmeow added 2 commits February 19, 2026 12:58

Merge trunk

7d35b6f

Test cleanup

f32d6e3

jonmeow approved these changes Feb 19, 2026

View reviewed changes

jonmeow enabled auto-merge February 19, 2026 21:06

jonmeow added this pull request to the merge queue Feb 19, 2026

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 19, 2026

jonmeow added this pull request to the merge queue Feb 19, 2026

Merged via the queue into carbon-language:trunk with commit fdb188c Feb 20, 2026
8 checks passed

Conversation

burakemir commented Dec 17, 2025

Uh oh!

jonmeow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

burakemir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonmeow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonmeow commented Dec 18, 2025

Uh oh!

jonmeow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

burakemir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

burakemir commented Feb 13, 2026

Uh oh!

jonmeow commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

burakemir commented Feb 16, 2026

Uh oh!

jonmeow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonmeow Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

burakemir Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

burakemir Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonmeow Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

burakemir Feb 18, 2026

jonmeow commented Feb 13, 2026 •

edited

Loading

jonmeow left a comment •

edited

Loading

burakemir Feb 18, 2026 •

edited

Loading

burakemir Feb 18, 2026 •

edited

Loading