--hintAsError:X (analog to --warningAsError:X)

[timotheecour]

refs #15423 (comment)

this PR makes it easier to fix code so that hints such as ConvFromXtoItselfNotNeeded are not generated; this flag can be used in CI in future work (either nim's CI or in other package's CI)

(There are other flags one may want to turn into an error in one's CI/cleanups beyond ConvFromXtoItselfNotNeeded, and other uses of hintAsError too)

note

needed for followup PR #16764

[Clyybber]

IMO this should be reverted until it has an RFC,
because right now it's not clear that it has any reasonable usecase; how it should interact with other switches (--verbosity:2); how many cans of worms it opens (--hintAsWarning??); and most importantly the potential damage it can cause to the ecosystem if people start putting this switch into their configs and then request package authors to change their code to work with the newly created dialect.

[timotheecour]

IMO this should be reverted until it has an RFC,

A revert isn't justified IMO but I'm happy to discuss any further concern.

because right now it's not clear that it has any reasonable usecase

it's the exact same rationale as warningAsError. For example, it allows one to turn select hints into errors which can be used in CI to prevent regressions of such hints re-appearing, see for example #16764.

how it should interact with other switches (--verbosity:2)

the way you'd expect; --verbosity enables specific hints and warnings; warningAsError:X and hintAsError:Y enables hint X / warning Y and turns it in an error. Nothing new here, both warningAsError and hintAsError work in conjunction with verbosity.

how many cans of worms it opens (--hintAsWarning??)

I wouldn't propose that, it wouldn't help with CI.

and most importantly the potential damage it can cause to the ecosystem if people start putting this switch into their configs and then request package authors to change their code to work with the newly created dialect.

it's the exact same with warningAsError and no-one complained about it, I don't see a problem there.

[nc-x]

it's the exact same with warningAsError and no-one complained about it, I don't see a problem there.

It is not the same as warningAsError because warnings mean that there is something that could potentially cause issues further down the line, whereas hints are, well, hints and as such have nothing to do with errors. And if currently there is any hint that signifies potential danger, it should be converted to a warning IMO.

In any case, I am fine with both reverting this PR or leaving it be, it is not a big issue i think.

[timotheecour]

warnings mean that there is something that could potentially cause issues further down the line, whereas hints are, well, hints and as such have nothing to do with errors

the boundary between warning and error is a gray area, some hints could be considered warnings and vice versa. If your definition of warning is potentially cause issues further down the line, then many warnings shouldn't actually be warnings.

In particular, there's not much severity difference between these 2 groups:

• warnCommentXIgnored, warnUnusedImportX, warnUnreachableElse, warnUnreachableCode, warnUninit, warnResultShadowed, warnInconsistentSpacing

• hintPerformance, hintLineTooLong, hintXDeclaredButNotUsed, hintXCannotRaiseY, hintConvToBaseNotNeeded, hintConvFromXtoItselfNotNeeded, hintExprAlwaysX, hintConditionAlwaysTrue, hintConditionAlwaysFalse, hintPerformance

And if currently there is any hint that signifies potential danger, it should be converted to a warning IMO.

this would be a breaking change.

So instead of converting some warnings to hints or vice versa (and debating forever which one is which), which would introduce a breaking change, I took the much more practical route of --hintAsError:X.

[Clyybber]

this would be a breaking change.

No it would not. Getting a warning instead of a hint cannot break your code if that warning didn't exist before (because #14068 is not merged, for good reason).

it's the exact same with warningAsError and no-one complained about it, I don't see a problem there.

It's not the same, warnings have a bad connotation, package authors are inclined to get rid of them even without --warningAsError. Hints on the other hand do not have a negative connotation so theres no reasonable reason to silence or get rid of them.

[timotheecour]

No it would not.

I was referring to the suggestion And if currently there is any hint that signifies potential danger, it should be converted to a warning IMO. If instead of converting hintX to warnX you add a warnX (and keep hintX), you end up with more complexity to handle for end users especially for those who must make their code work with multiple versions of nim.

Hints on the other hand do not have a negative connotation

like I said, this distinction in severity can be arbitrary/historical in many cases, look at the 2 lists of hints/warnings I gave in #16763 (comment), I doubt there would ever be a consensus on which one should've been a hint vs a warning; hintAsError:X avoids this pointless philosophical question.

[disruptek]

There are pretty obvious differences between the warnings and hints you mentioned; if anything, I'd move warnCommentXIgnored and warnUnusedImportX to hints, though I'm honestly unsure what the point is of the first warning.

However, the purpose of warnings and hints is different. To conflate them is an error, as is this PR.

[Clyybber]

I was referring to the suggestion And if currently there is any hint that signifies potential danger, it should be converted to a warning IMO. If instead of converting hintX to warnX you add a warnX (and keep hintX), you end up with more complexity to handle for end users especially for those who must make their code work with multiple versions of nim.

Converting hintX to warnX is not a breaking change, the corresponding --hint[X] switch can be kept as a noop.

like I said, this distinction in severity can be arbitrary/historical in many cases, look at the 2 lists of hints/warnings I gave in #16763 (comment), I doubt there would ever be a consensus on which one should've been a hint vs a warning; hintAsError:X avoids this pointless philosophical question.

It avoids the question, but it makes the situation worse by conflating them even more, as the presence of a --hintAsError switch gives the impression that hints are something to be avoided.

[timotheecour]

if anything, I'd move warnCommentXIgnored and warnUnusedImportX to hints

this would prevent valid use cases such as some codebase adding to their CI a check against unused imports.

warnings mean that there is something that could potentially cause issues further down the line

With the same logic, --styleCheck:error would not exist.

No-one's forcing you to use hintAsError, warningAsError, styleCheck, etc; these are just tools you can use to maintain certain guarantees in your code, in particular in CI.

[disruptek]

I don't remember defending styleCheck:error, but since you brought it up, yes, it's stupid, too.

If you want to crash your CI, this is easy enough to do using grep. By adding these features to the compiler itself, you ensure that the already difficult job of parsing configuration options and running standardized tests across the ecosystem is made even harder by fascist developers. We need less of this crap, not more.

[Araq]

like I said, this distinction in severity can be arbitrary/historical in many cases, look at the 2 lists of hints/warnings I gave in #16763 (comment), I doubt there would ever be a consensus on which one should've been a hint vs a warning; hintAsError:X avoids this pointless philosophical question.

That is the argument that I considered when I merged the PR and I still find it more convincing than the other arguments. However, I consider warningAsError to be the mistake, but all the other big players (clang, etc) tend to have it... More importantly, warnings which we can later turn into errors allow us to evolve Nim more easily. For example, I want enforced initialization and better not-nil checking and warnings will get us there.