Fix Issues 20718, 15867, 12380 - Wrong location errors on const. folding / exp. optimization

[BorisCarvajal]

A rude attempt to fix most of error messages with the wrong line when there are constant exp. rewrites, this also stops the special case handling of them. (I remember having seen a few exp.loc repainting).

fail_compilation/diag9250.d was removed to see if everything else passes, and possibly discuss its validity.

[dlang-bot]

Thanks for your pull request and interest in making D better, @BorisCarvajal! 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
✓	12380	normal	Wrong line number for type mismatch with enum .init assignment
✓	15867	minor	Compiler reports wrong error location for immutability error
✓	20718	regression	enum type mismatch causes wrong location on error

Testing this PR locally

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

dub run digger -- build "stable + dmd#11002"

[MoonlightSentinel]

What's the problem with fail_compilation/diag9250.d?

[schveiguy]

I think there's nothing invalid about diag9250.d. That's exactly the errors I would expect to see.

[BorisCarvajal]

Yep, diag9250.d is just fine I was overthinking about its failure.

And I discarded this approach in favor of something less invasive that can't silently break other uses.
So I patched the expression location on expansion to struct literal members and constants and so far so good the test passes, but there is a gotcha in the case of a variable interpreted to a const initializer, I'm overwriting the original expression location such that it always points to the last expansion line, e.g.

static immutable s = "a"; // "a" location -> line 1
static immutable x1 = s;  // "a" now is in line 2
static immutable x2 = s;  // and line 3 after here
(Note: 's' location is fine)

This is apparently harmless yet feels inelegant, and the alternative is maybe to make a copy of the expression however it's allocating more memory for a better error message that in a 99.9% will never going to happen.
On struct literal member the compiler seems to always give a new expression thus setting the location doesn't modify the original.

[schveiguy]

there is a gotcha

What does this mean in practice? The thing I don't want is to have the complier point at the wrong line for an error message. If we fix this bug, but introduce another for a different error, that's not progress.

[BorisCarvajal]

I'm sure I wanted to write "there is a catch".

If we fix this bug, but introduce another for a different error, that's not progress.

I'll investigate if that's the case.

[schveiguy]

Thanks, and pardon my ignorance on the compiler internals. I can't really review it, but I can just ask questions as a layman user.

[WalterBright]

From the code changes this looks like two separate bug fixes. Please do ONE PR per issue, unless the issues are duplicates.

The reason is simple. I don't like dealing with regressions, and if a PR that does multiple independent things causes a regression, it makes it much harder to isolate and fix. Another reason is one fix may be acceptable and the other not - the beleaguered reviewer shouldn't have to hold up the good one because of the bad one. I can go on ...

Merging multiple issues into one PR does NOT save time, and there's no reason to conserve PR numbers.

[WalterBright]

Break into 2 PRs because it's two different fixes.

[schveiguy]

I marked the issues as duplicates. The fix is small, so I don't know how you split it up.

[schveiguy]

Yep, diag9250.d is just fine I was overthinking about its failure.

And I discarded this approach in favor of something less invasive that can't silently break other uses.

It seems like you have made changes but not pushed them?

[thewilsonator]

I think its more important to have this than split this in two PRs.

Need to get things done.

[BorisCarvajal]

This really shouldn't have been merged and must be reverted.

The PR splitting is not the only cause to not merge it. I replied above that I discarded the current approach in favor of something else that I haven't finished/decided.
As it was blocked by @WalterBright I kept the thing open to get back on it in the future.

Sorry I took a break on this issue, not sure when I'll resume it.