FSharpDiagnostic: add default severity #19152
Conversation
❗ Release notes required
|
Martin521
left a comment
Martin521
left a comment
There was a problem hiding this comment.
That makes a lot of sense.
The whole diagnostics logging area is so over-complicated. I welcome every step to simplify it.
| | FSharpDiagnosticSeverity.Hidden -> [] | ||
| | adjustedSeverity -> | ||
|
|
||
| let diagnostic = { diagnostic with Severity = adjustedSeverity } |
There was a problem hiding this comment.
Without reading the implementation, I had problems imagining what default meant in the context of F.C.S public API.
Did you consider other names?
OriginalSeverity or UnadjustedSeverity perhaps?
There was a problem hiding this comment.
I think the current naming makes sense. Maybe an XmlDoc would be sufficient? We could then leave the current name. If not, I'd vote for OriginalSeverity.
There was a problem hiding this comment.
PhasedDiagnostic is not part of the public API, so I would keep it here as is.
See my comment below for the public API.
There was a problem hiding this comment.
For example, in Roslyn it is also known as 'DefaultSeverity'
However, as for me, the documentation for this is even less informative than in this PR
| | FSharpDiagnosticSeverity.Hidden -> [] | ||
| | adjustedSeverity -> | ||
|
|
||
| let diagnostic = { diagnostic with Severity = adjustedSeverity } |
There was a problem hiding this comment.
PhasedDiagnostic is not part of the public API, so I would keep it here as is.
See my comment below for the public API.
| member Severity: FSharpDiagnosticSeverity | ||
|
|
||
| /// Gets the default severity, does not take into account diagnostics options | ||
| member DefaultSeverity: FSharpDiagnosticSeverity |
There was a problem hiding this comment.
I don't think the original severity should be publicly accessible. It is an implementation detail. The adjusted severity is the specified output of the compiler (after applying the different flags and directives) and should be the only visible one.
There was a problem hiding this comment.
The default severity levels (whether semantic treats a diagnostic as an error or a warning) for compiler diagnostics are publicly available and not determined by any special internal logic. In practice, they are also unlikely to change.
The FSharpDiagnostic API is used not only during compilation but also by the editor services. Therefore, it provides valuable information that enhances the user experience in the IDE — for example, the ExtendedData property, which allows the IDE to access additional diagnostic details for implementing quick fixes. And it would be nice to know not only how to display diagnostics, but also how significant they are: #nowarn can disable warnings promoted to errors, unlike real errors.
4 participants
This API allows the IDE to differentiate compiler errors from warnings. Useful, for example, for suggesting actions like adding a #nowarn directive.