Fix issue 20488: opDispatch: copy call expression before checking for semantically valid parameters

[FeepingCreature]

This may avoid accidentally resolving future occurrences to the wrong type, not sure.
Fixes issue 20488.
This relates to my PR from August 18 regarding invalid opDispatch parameters being a first-line syntax error rather than a (silent) opDispatch failure: #8584 .

Somehow the opDispatch call ends up using the _aaLen from the bool[int] case? I don't fully understand what happens here, but we need to clone the CallExp anyway, so just doing it before the semantic check on the arguments is probably the way to go.

[dlang-bot]

Thanks for your pull request and interest in making D better, @FeepingCreature! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	20488	regression	AA.length in multiple modules causes opDispatch failure

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + dmd#10718"

[thewilsonator]

Please target stable.

[FeepingCreature]

Rebased to stable.

[FeepingCreature]

Okay, this failure is bizarre. It's indirectly caused by simply calling CallExp.syntaxCopy(), even if I discard the result. What the hell...?

[FeepingCreature]

Worked around (?) by avoiding copying ce.e1 too early - now we just copy the arguments, which fixes the bug, and copy the replacement e1 later.

[Geod24]

@thewilsonator : I think this needs a proper review before applying the auto-merge again.

[FeepingCreature]

Yeah, also I still need to look into whether ce.e1 = was deliberately meant to apply to the fallthrough. Anyone who can shed light on how that opDispatch gymnastics is meant to work: please advise.

[FeepingCreature]

Okay, I've changed the PR to the smallest set that I think can conceivably work. It's a bit memory wasteful now (a somewhat unneeded (?) syntax copy); not sure how to manage that.

[FeepingCreature]

Bleh.

This is annoying. Any way I slice it, I trip over some unexpected consequence or bug.

Basically, the problem is with syntax like a.b(c), we try to resolve this with opDispatch on a, then if that fails, we try UFCS. So we have to suppress errors created by the opDispatch missing, leading to the "no such member: foo" message in 19181. However, the opDispatch being called with invalid parameters should still immediately cause it to error out. However, the parameter for the opDispatch call is evaluated deeper into UnaExp::semantic, and I can't tell it "suppress any issues with the template instantiation, but not with the parameters" without semanticing the parameters first, and I can't do that without causing problems with later failures, and I can't copy the parameters without running into unrelated issues I don't understand, aside from it just wasting memory, and I can't run semantic on the parameters first because I can't stop UnaExp::semantic from trying to run semantic on them a second time.

[FeepingCreature]

Well, let's try it like this:

Instead of trying the parameters out upfront, why not just run the check only if we've already seen some indication of a failure (within the gagged expressionSemantic)? After all, the problem in 19181 wasn't a missing error but rather a wrong one.

This shouldn't do anything differently than before; just in a more trustworthy order. The problem is side effects from semantic colliding (I think), so only running the second semantic if the first has already failed should at least make things more stable. I don't think this is a perfect fix, but (if it works) it should at least be an improvement. I think we can still get a semantic problem if there's a side effect of a semantic step that's run before the call parameter that caused the gagged call to error, but I'm not sure how to fix that without completely restructuring the way semanticExpression(CallExp) works.

---

edit: Oh and make another god damn copy of the original arguments in this case, so you get a proper error message instead of double-eating __error! Blagh.

[FeepingCreature]

ping @Geod24 @thewilsonator it seems to work now??

[FeepingCreature]

ping