[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

Thu Jul 20 11:46:04 PDT 2023

aaron.ballman added a comment.

In D150226#4516337 <https://reviews.llvm.org/D150226#4516337>, @dblaikie wrote:

> In D150226#4515843 <https://reviews.llvm.org/D150226#4515843>, @aaron.ballman wrote:
>
>> In D150226#4515783 <https://reviews.llvm.org/D150226#4515783>, @dblaikie wrote:
>>
>>> In D150226#4498024 <https://reviews.llvm.org/D150226#4498024>, @aaron.ballman wrote:
>>>
>>>> In D150226#4461846 <https://reviews.llvm.org/D150226#4461846>, @efriedma wrote:
>>>>
>>>>> The fundamental problem here is the interaction with SFINAE; if we don't diagnose this as an error, we actually miscompile some cases.  Because of that, a flag wouldn't really be "turning off the warning"; it would be enabling a non-compliant language mode that has different rules for whether enum casts are legal.
>>>>>
>>>>> If it weren't for that, I don't think anyone would be in a hurry to turn the warning into a hard error.
>>>>
>>>> +1 to this. I'm worried about the miscompiles continuing in the wild. We did the best we could when landing the warning-as-error changes in D131307 <https://reviews.llvm.org/D131307> by telling users explicitly "This diagnostic is expected to turn into an error-only diagnostic in the next Clang release." in the release notes. Obviously, that's not ideal because very few folks read release notes, but we don't have any better mechanism for communicating these kinds of changes.
>>>>
>>>> What do folks think is a reasonable path forward here? Given that we're going to branch the Clang 17 release in a few weeks (AIUI), my suggestion is to kick the can down the road by landing these changes just after we branch. That way the changes land such that early adopters can test and see how disruptive we're being without time pressure or hassle of dealing with release branches. Do folks think that's a reasonable approach? (I'm open to other suggestions.)
>>>
>>> I'm not quite following if/what the objection is to removing the "ignore in system headers" for the warning for a release or two prior to making this a hard error? That seems like a fairly low-cost change (it does kick the can down the road a release or two before it becomes a hard error - but isn't worse for users, for the most part - if they were going to get a hard error from a system header before, at least now instead they'd get a warning or warning-defaulted-to-error instead, they could turn it off (but they had been warned that their system headers were going to cause them problems soon) and then it becomes a hard error a release or two later).
>>
>> Thank you for bringing that up, sorry for not being more explicit -- by "these changes", I meant "whatever form we decide they should take" as opposed to "the patch as it is now."
>
> Ah, OK.
>
>> In terms of removing the "ignore in system headers" flag, I'm not strongly opposed to it, but I do worry it will throw the baby out with the bathwater. The specific use case I'm worried about is: user has a system header with the problematic code, they run into this diagnostic and they can't address it themselves so they disable the warning for the project. The user then doesn't learn about the problems in their own (non-system header) code until it becomes a hard error later.
>
> OK, I think we're still talking past each other/making different comparisons.
> I think you're comparing "make this a warning in system headers" to "not doing anything" (so, yes, it causes people to ignore the warning even in their own code when if they had done nothing/we had done nothing to force them, they would've kept getting the warning in their code)
> Whereas I'm comparing "make this a warning in system headers" to "making this a hard error" (in which case a user for which it would've been a hard error and they'd have had no choice but to fix their system headers (might be hard/impossible) would have some relief valve for a time/some notice that this would be a problem in the future when it becomes a hard error)
>
> If we picture a timeline:
> Time 1) Code is valid/no problem
> Time 2) Warning added (not in system headers)
> Time 3) Warning becomes error by default (but could be disabled by a user) (still not in system headers)
> Time 4) Becomes hard error
>
> And if someone has a system header with a violation, they won't know about it until (4) and they won't be able to do much/anything about it.
>
> I'm suggesting making the process longer.
>
> Time 4) default-error warning becomes enabled in system headers
>
> Users who, in the first timeline, would be stuck - at least have a release valve. I think they are better off than they were in the first timeline - even though they lose warning coverage over their own code (which is bad), at least they can still build their project and know they have problems in their system headers they should report/etc and try to have addressed.
>
> then Time 5) make it a hard error.
>
> So it takes a bit longer to get to (5), and users who would be OK in the original timeline are no worse off - they had there system headers fixed by (4) and don't need to disable the warning. But users who would've had a bad time at (4) have a less bad time/some chance of working through the bad time.

Thank you for the detailed explanation, that's really helpful! I think we were approaching this from somewhat different angles, but I now better understand what you mean. The tradeoff boils down to a question of which is less bad for users: losing warning coverage in their own code or finding out they have to get new system headers (or modify the existing ones, but no easy switch to address the issue). I think getting new system headers is harder for projects to deal with, so I think I'm now in agreement with you that we should enable the warning as an error in system headers.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150226/new/

https://reviews.llvm.org/D150226