Fix with tuple only processing first binding in build_with_dispose

[cuiweixie]

Summary

The loop in build_with_dispose returned on the first recursive call, so only the first tuple element was processed. Later _ patterns were not diagnosed as errors, and dispose calls were only added for the first binding.

Changes

• Fix the tuple branch to iterate all elements and propagate failure.
• Add WithTest.DontcareInTupleSecondBindingIsRejected in test/libponyc/with.cc.

Merge commit message

Fix with tuple only processing first binding in build_with_dispose

The loop returned on the first recursive call, so later tuple elements
were skipped: `_` in a later position was not rejected, and dispose
calls were only emitted for the first binding.

Walk every element and propagate failure. Add a libponyc regression test.

[ponylang-main]

Hi @cuiweixie,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 5077.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

[SeanTAllen]

The test covers the dontcare diagnostic, but the other half of the bug (dispose calls only emitted for the first binding) doesn't have coverage. A compile-success test with a valid two-element tuple with block would close that gap. Something like with (a, b) = (D.create(), D.create()) do None end compiling cleanly proves the loop runs all the way through on the happy path too.

[SeanTAllen]

can you update this to show an example of pony code that failed due to the bug? we like to show an example of what previously didn't work but does now in our release notes. thanks.

[SeanTAllen]

Sorry I didn't catch this in my first pass. This should use the error accumulation pattern instead of early return. There's a precedent in sugar_match_capture around line 690 in this same file:

bool r = true;
for(ast_t* p = ast_child(idseq); p != NULL; p = ast_sibling(p))
{
  pony_assert(ast_id(p) == TK_SEQ);
  ast_t* let = ast_child(p);
  if(!build_with_dispose(opt, dispose_clause, let))
    r = false;
}
return r;

With early return, with (_, _) = ... only reports the first error. The user has to fix, recompile, discover the next one. The accumulation pattern reports all of them at once. Same fix, better UX.

[SeanTAllen]

If you switch to the accumulation pattern, add a test for (_, _) that expects two errors. That's the case where early return hides the second diagnostic.