[Diagnostics] Add -print-diagnostic-groups flag #76363
Conversation
DmT021
requested review from
a team,
hborla,
slavapestov,
xedin,
ktoso,
zoecarver,
hyp,
egorzhdan,
ahoppen,
bnbarham,
hamishknight,
rintaro,
artemcm,
tshortli,
eeckstein and
DougGregor
as code owners
|
@DougGregor please take a look I've also added the flag to many relevant tests. This way we test that the messages we're getting are expected and belong to the expected group. IMO it's nice to have some extra coverage so we can be sure we don't change the group of a warning accidentally. But let me know if you think otherwise. Also, there's a use of |
DmT021
force-pushed
the
wp/print-diagnostic-groups
branch
from
c36dfce to
23c57d3
Compare
| @@ -1518,7 +1512,7 @@ DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic) { | |||
|
|
|||
| return DiagnosticInfo( | |||
| diagnostic.getID(), loc, toDiagnosticKind(behavior), | |||
| diagnosticStringFor(diagnostic.getID(), getPrintDiagnosticNames()), | |||
| diagnosticStringFor(diagnostic.getID(), getPrintDiagnosticNamesMode()), | |||
| diagnostic.getArgs(), Category, getDefaultDiagnosticLoc(), | |||
There was a problem hiding this comment.
It looks like Category here has a few hardcoded values. Should we provide the warning group name for Category when there is one?
There was a problem hiding this comment.
I think we probably could do it. But I'm sure we should. These categories are inferred from DiagnosticOptions.
The deprecation category seems to be interesting to look at. Maybe we should consider removing the DiagnosticOptions::Deprecation option and instead infer this category from checking if a diagnostic belongs to the deprecated group. So we don't have two ways to express the same thing.
And I don't know if we need the api-digester-breaking-change and no-usage categories expressed as groups.
There was a problem hiding this comment.
I took a look at the diagnostics that get each of these flags: the "deprecation" category does seem to line up fairly well with the "deprecated" group, the "no-usage" category could be useful as a general diagnostic group for "you did something and didn't use the result" warnings, and "api-digester-breaking-change" is a very specific group of separate diagnostics (mostly errors).
|
Only a small comment and a question, this looks good! |
|
@swift-ci please smoke test |
|
@DougGregor Since there are changes in the public members of I don't know how to deal with it, both of these PRs depend on each other and should be tested and merged together. Can you rerun the tests here with that PR? |
DmT021
force-pushed
the
wp/print-diagnostic-groups
branch
from
23c57d3 to
36a7417
Compare
|
@swift-ci please smoke test |
Yes, of course. I've kicked off the multi-PR test, sorry I missed that earlier. |
This change adds the `-print-diagnostic-groups` flag as described by SE-0443.
DmT021
force-pushed
the
wp/print-diagnostic-groups
branch
from
36a7417 to
a8b71ea
Compare
|
@DougGregor The windows tests failed because of the missing quote marks in |
|
@swift-ci please smoke test |
This change adds the
-print-diagnostic-groupsflag as described by SE-0443.