I filed issue 17635 [1] in 2017. A low-quality fix was merged later that year.

In 2018, Walter moved the test case for issue 17635 from /compilable/ to /fail_compilation/ [2]. I.e., he flipped it upside down. He provided no explanation. He did not update the issue in Bugzilla. I only found out about it now, because I'm working on a related issue and that test failed when I tried my fix.

The pull request was approved by Jacob Carlborg. He also had nothing to say about the flipping of the test case. But the branch was force-pushed eight times after the approval, so it's possible that the problematic change wasn't part of the version that Jacob reviewed. In which case, the approval should have been considered outdated.

If I were unreasonably amicable, I might say that Walter simply forgot to go back to Bugzilla and update the issue. But even then he should have mentioned in the pull request that it affects an older issue. And having dealt with Walter before, I'm not willing to find excuses anymore. I'd sooner believe that he snuck that change past Jacob with a force-push.

Changes to pre-existing tests must be done very carefully. When a pull request replaces a test with its exact opposite, that should be a big red flag that must be well justified.

Sadly, I can't be surprised that this happened. But I am pissed.


[1] https://issues.dlang.org/show_bug.cgi?id=17635
[2] https://github.com/dlang/dmd/pull/8048

On Monday, 19 July 2021 at 10:54:47 UTC, ag0aep6g wrote:
> [...]
>
> If I were unreasonably amicable, I might say that Walter simply forgot to go back to Bugzilla and update the issue. But even then he should have mentioned in the pull request that it affects an older issue. And having dealt with Walter before, I'm not willing to find excuses anymore. I'd sooner believe that he snuck that change past Jacob with a force-push.
>
> [...]

in the last force push it seems the file was moved:

https://github.com/dlang/dmd/compare/9e598d64c076d8b8c2f79f0214cddf3d55f75e11...ab5a18635adaa3e2271e0f4b569500a89ac74235

I haven't really thought about the content yet though, it might be that this could actually be a more correct behavior, which might be why Walter did this change.

On 19.07.21 16:55, WebFreak001 wrote:
> in the last force push it seems the file was moved:
> 
> https://github.com/dlang/dmd/compare/9e598d64c076d8b8c2f79f0214cddf3d55f75e11...ab5a18635adaa3e2271e0f4b569500a89ac74235 

I'm not sure I know how to read these things, but it looks bad to me. AFAICT, this is the version that was reviewed:

https://github.com/dlang/dmd/commit/0b75c4567c012cc865d3e07ac50c08d59cca1800 (2018-03-18T01:28:36Z)

fix17635.d was not touched in that version.

Jacob approved at 2018-03-18T11:36:52Z.

And this is the oldest version I can see that messes with fix17635.d:

https://github.com/dlang/dmd/commit/380ecff8fe281cff246b1b32c2e965a2691bd063 (2018-03-19T21:40:04Z)

So it looks like Walter pushed non-trivial updates after the approval. And then he merged his own pull request based on the outdated approval.

> I haven't really thought about the content yet though, it might be that this could actually be a more correct behavior, which might be why Walter did this change.

It's not more correct. But even if it were, Walter would need to (1) explain that and (2) update the Bugzilla issue accordingly (mark invalid instead of fixed).