Add diagnostic warning levels (-Wall/-Wextra/-Wpedantic)#11812
Add diagnostic warning levels (-Wall/-Wextra/-Wpedantic)#11812expipiplus1 wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds warning-level grouping across the diagnostic API, Lua diagnostics pipeline, CLI option handling, sink filtering, and validation tests. It tags ChangesWarning Level Groups
Related issue: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds clang/gcc-style diagnostic warning groups to Slang, allowing opt-in “advisory/stylistic” warnings via -Wall / -Wextra / -Wpedantic (and the corresponding public compiler option), while keeping untagged warnings always-on by default.
Changes:
- Introduces a public
SlangWarningLevelandCompilerOptionName::WarningLevel, plus an internalWarningLevelcarried onDiagnosticInfo. - Implements warning-group gating in
DiagnosticSink::getEffectiveMessageSeverity, while preserving per-diagnostic overrides (-W…/-Wno-…) precedence. - Adds CLI parsing for
-Wall/-Wextra/-Wpedanticand an end-to-end regression test covering group gating and override behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
include/slang.h |
Adds public SlangWarningLevel and new CompilerOptionName::WarningLevel. |
source/compiler-core/slang-diagnostic-sink.h |
Adds internal WarningLevel, DiagnosticInfo::level, and sink enable/query helpers + bitmask. |
source/compiler-core/slang-diagnostic-sink.cpp |
Implements group gating in effective severity resolution; adjusts override retention logic for grouped warnings. |
source/slang/slang-diagnostics-helpers.lua |
Adds/export warning-level sentinels and propagates level through diagnostic processing. |
source/slang/slang-diagnostics.lua |
Imports sentinels and tags implicit-conversion-to-double as pedantic. |
source/slang/slang-rich-diagnostics.h.lua |
Adds mapping from Lua warning-level strings to C++ WarningLevel enums. |
source/slang/slang-rich-diagnostics.cpp |
Emits the warning level into generated DiagnosticInfo initializers for rich diagnostics. |
source/slang/slang-options.cpp |
Registers -Wall/-Wextra/-Wpedantic and maps spellings to SlangWarningLevel option values. |
source/slang/slang-compiler-options.cpp |
Applies the (repeatable) WarningLevel option array to the diagnostic sink. |
tests/diagnostics/warning-levels.slang |
Adds an end-to-end test covering default suppression, group enable, per-id enable, and -Wno precedence. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/slang/slang-diagnostics-helpers.lua (1)
392-406: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle warning-level sentinels before binding
primary_span.Locationless diagnostics are allowed, but this parser only recognizes
is_warning_levelinside.... A call likewarning("foo", 1, "msg", pedantic)leavespedanticinprimary_span, so the diagnostic stays in the default group and the entry shape becomes invalid.Suggested fix
local function add_diagnostic(name, code, severity, message, primary_span, ...) + local level = "default" + if primary_span and primary_span.is_warning_level then + level = primary_span.level + primary_span = nil + end + local diag = { name = name, code = code, severity = severity, message = message, primary_span = primary_span, - -- Warning group; "default" (always emitted) unless a sentinel below overrides it. - level = "default", + -- Warning group; "default" (always emitted) unless a sentinel overrides it. + level = level, } @@ for _, s in ipairs(extra_spans) do if s.is_warning_level then + if diag.level ~= "default" then + error("diagnostic '" .. name .. "' specifies more than one warning level") + end -- A warning-level sentinel (all/extra/pedantic): record the group and skip it; it is -- not a span or note. diag.level = s.level
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0a0eee7-0b3b-4153-a1d5-11aab52b2104
📒 Files selected for processing (10)
include/slang.hsource/compiler-core/slang-diagnostic-sink.cppsource/compiler-core/slang-diagnostic-sink.hsource/slang/slang-compiler-options.cppsource/slang/slang-diagnostics-helpers.luasource/slang/slang-diagnostics.luasource/slang/slang-options.cppsource/slang/slang-rich-diagnostics.cppsource/slang/slang-rich-diagnostics.h.luatests/diagnostics/warning-levels.slang
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9348627a-3da5-41e5-b001-19e6e92f1644
📒 Files selected for processing (3)
source/compiler-core/slang-diagnostic-sink.hsource/slang/slang-diagnostics.luatests/diagnostics/warning-levels.slang
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2b78215-0dc9-4f19-92d6-30440a33fe33
📒 Files selected for processing (6)
docs/user-guide/08-compiling.mdsource/compiler-core/slang-diagnostic-sink.hsource/slang/slang-compiler-options.cppsource/slang/slang-diagnostics.luatests/diagnostics/warning-levels.slangtools/slang-unit-test/unit-test-diagnostic-warning-level.cpp
Add clang/gcc-style warning groups to the diagnostic system. Each group is enabled independently: a warning tagged with a group is emitted only when that group is enabled, while untagged warnings stay in the always-on default group. Warnings are tagged in slang-diagnostics.lua by passing an `all`/`extra`/ `pedantic` sentinel positionally to warning(); the level rides on DiagnosticInfo and is gated in DiagnosticSink::getEffectiveMessageSeverity. Explicit per-id overrides (-W<name>/-Wno-<name>) still take precedence over group gating. Exposed via the -Wall/-Wextra/-Wpedantic CLI flags and the WarningLevel compiler API option (SlangWarningLevel). As an example, implicit-conversion-to-double (E30082), a purely advisory performance hint, is moved to the pedantic group. Fixes shader-slang#11012.
… default Switch the example pedantic warning from implicit-conversion-to-double to keyword-used-as-name (E20103): using a type keyword as a name is legal and usually works, so it is a style/portability hint rather than a likely bug. Enable the pedantic group by default for now so this grouping does not change any current output; a follow-up will flip pedantic to off-by-default and assign groups to the rest of the catalog.
…s/docs - Bounds-check the bit shift in enableWarningLevel/isWarningLevelEnabled and validate the WarningLevel int at the applySettingsToDiagnosticSink boundary, so a bogus value cast in through the public option cannot cause an out-of-range shift (undefined behavior). - Add CompilerOptionName::WarningLevel to allowDuplicate() so -Wall/-Wextra/ -Wpedantic accumulate instead of collapsing to one stored group. - Drop the unused all/extra Lua aliases (keep pedantic). - Document WarningLevel and the -Wall/-Wextra/-Wpedantic flags in the user guide. - Add a DiagnosticSink unit test covering off-by-default group gating, per-id override precedence, the warnings-as-errors interaction, and out-of-range safety; exercise -Wall/-Wextra in the end-to-end test.
- Note in the user guide that the pedantic group is enabled by default today (so -Wpedantic is currently a no-op), pending the follow-up that makes it opt-in. - Use a direct include path (compiler-core/slang-diagnostic-sink.h) in the new unit test instead of relative traversal.
The parent-sink constructor forwarded flags/color/unicode but not the enabled warning-group bitmask, so a child sink lost the group configuration. Add get/setEnabledWarningLevels (mirroring getFlags/setFlags) and copy it too.
6bf0db4 to
a90b37a
Compare
Exercises the additive allowDuplicate handling and the exact-match-wins disambiguation against the -W<id> prefix in a single slangc run.
The check-cmdline-ref CI step compares docs/command-line-slangc-reference.md against `slangc -help-style markdown -h`; the new warning-level flags need the generated reference updated.
…-Wpedantic staging in help string
There was a problem hiding this comment.
Verdict: 🟡 Minor — 1 gap
Adds clang/gcc-style warning groups (WarningLevel::Default/All/Extra/Pedantic) to DiagnosticSink with -Wall/-Wextra/-Wpedantic CLI flags and a public CompilerOptionName::WarningLevel = 155. Gating lives in getEffectiveMessageSeverity; per-id -W<name>/-Wno-<name> overrides win over group gating (with a corresponding fix in overrideDiagnosticSeverity so a -W<name> restoring the nominal severity is not stripped for grouped warnings). Pedantic is on by default as a staging step; only keyword-used-as-name (E20103) is tagged so far.
Changes Overview
Diagnostic sink infrastructure (source/compiler-core/slang-diagnostic-sink.{h,cpp})
- New
WarningLevelenum withstatic_asserts mirroring the publicSLANG_WARNING_LEVEL_*.DiagnosticInfogains alevelfield (defaultDefaultfor aggregate-init compat).DiagnosticSinkgainsenableWarningLevel/isWarningLevelEnabled(bounds-check< 32before shift),get/setEnabledWarningLevels, anm_enabledWarningLevelsbitmask initialized withPedanticbit set, and parent-sink inheritance of the bitmask.getEffectiveMessageSeveritygates warnings after per-id overrides;overrideDiagnosticSeveritykeeps overrides equal to default severity when the diagnostic is in an opt-in group.
Option plumbing (source/slang/slang-compiler-options.cpp, slang-options.cpp, include/slang.h)
- New public
SlangWarningLevelenum andCompilerOptionName::WarningLevel = 155(appended beforeCountOf). CLI parser maps-Wall/-Wextra/-Wpedanticvia string comparison;applySettingsToDiagnosticSinkiterates the (allow-duplicate)WarningLeveloption array and callsenableWarningLevelfor values in{ALL, EXTRA, PEDANTIC}, silently ignoring others.
Lua diagnostic generation (source/slang/slang-diagnostics-helpers.lua, slang-diagnostics.lua, slang-rich-diagnostics.h.lua, slang-rich-diagnostics.cpp)
- Positional
all/extra/pedanticsentinels inwarning()/err()setdiag.level;add_diagnosticrecognises them viais_warning_level. Rich-diagnostics template emitsWarningLevel::Xas the fifthDiagnosticInfoinitializer via a newgetWarningLevelEnumhelper that errors on unknown level names.
Tests (tests/diagnostics/warning-levels.slang, tools/slang-unit-test/unit-test-diagnostic-warning-level.cpp)
- Six unit tests: default gating, additive enabling, per-id override precedence, warnings-as-errors interaction, parent-sink inheritance, out-of-range safety. One
.slangfile with five slangc runs (default,-Wpedantic,-Wall,-Wextra, all-three,-Wno-<id>).
Docs (docs/command-line-slangc-reference.md, docs/user-guide/08-compiling.md)
- New CLI reference section and new
WarningLevelrow in theCompilerOptionValuetable.
Findings (1 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | docs/command-line-slangc-reference.md:260-266 |
File is auto-generated; hand-authored section diverges from what slangc -help-style markdown -h produces from the registration in slang-options.cpp. |
Fixes #11012.
Motivation
The diagnostic system had no notion of warning "levels": every warning was either always on, or had to be individually disabled with
-Wno-<id>. There was no way to group advisory/stylistic warnings the way clang/gcc do with-Wall/-Wextra/-Wpedantic.Proposed solution
Add clang/gcc-style warning groups. Following that model the groups are independent (not nested): a warning tagged with a group is emitted only when that group is enabled, while untagged warnings stay in the always-on default group. Most warnings need no annotation; only opt-in warnings are tagged.
A warning is tagged in
slang-diagnostics.luaby passing anall/extra/pedanticsentinel positionally towarning(), alongside its spans:The level rides on
DiagnosticInfoand gating happens in one place,DiagnosticSink::getEffectiveMessageSeverity. Explicit per-id overrides (-W<name>/-Wno-<name>/-warnings-as-errors) continue to take precedence over group gating.Control is exposed via the
-Wall/-Wextra/-WpedanticCLI flags and theWarningLevelcompiler-API option (SlangWarningLevel).Staging — pedantic is enabled by default for now. As the first example,
keyword-used-as-name(E20103) is taggedpedantic: using a type keyword as a name is legal and usually works, so it is a style/portability hint rather than a likely bug. To avoid silently dropping any warning from today's output, thepedanticgroup is enabled by default in this PR, so tagging only records the group and changes nothing observable yet. A follow-up will flippedanticto off-by-default and assign groups to the rest of the catalog.Change summary
include/slang.hSlangWarningLevelenum (Default/All/Extra/Pedantic) andCompilerOptionName::WarningLevel = 155(appended; intValue0 is the group to enable, repeatable).source/compiler-core/slang-diagnostic-sink.hWarningLevelenum (static-asserted against the public one), alevelfield onDiagnosticInfo(defaulted so existing aggregate/DIAGNOSTIC(...)initializers keep compiling),enableWarningLevel/isWarningLevelEnabled, and the enabled-groups bitmask (pedantic on by default for now).source/compiler-core/slang-diagnostic-sink.cppgetEffectiveMessageSeverity; keep an explicit "enable" override for grouped warnings (see Process report).source/slang/slang-diagnostics-helpers.luaall/extra/pedanticsentinels, recognise them inadd_diagnostic, carrylevelthrough to the processed entry, and export them.source/slang/slang-diagnostics.luakeyword-used-as-nameaspedantic.source/slang/slang-rich-diagnostics.h.lua/.cppgetWarningLevelEnummapping and emit the level as the 5thDiagnosticInfofield.source/slang/slang-options.cpp-Wall/-Wextra/-Wpedantic(one option, three names) and map each to aWarningLevelgroup value.source/slang/slang-compiler-options.cppWarningLeveloption array to the sink inapplySettingsToDiagnosticSink.tests/diagnostics/warning-levels.slang-Wnoprecedence.Concepts and vocabulary
all/extra/pedanticbucket a warning belongs to, mirroring the clang/gcc groups.Defaultis the implicit always-on group.DiagnosticInfo: the static descriptor for each diagnostic (id, severity, name, message — now alsolevel). The Lua catalog generates these for the rich diagnostics; the compiler-coreDIAGNOSTIC(...)macro catalogs generate the rest.getEffectiveMessageSeverity: the single chokepoint (used by both the legacy and rich diagnostic paths) that resolves a diagnostic's nominal severity into its effective severity after pragmas, per-id overrides, group gating, and warnings-as-errors.m_severityOverrides): the map behind-W<name>/-Wno-<name>/-warnings-as-errors, keyed by diagnostic id.Process report
Where the level lives, and why. The Lua catalog is the single source of truth for diagnostics, so the group is declared there and flows to
DiagnosticInfo::level. The sink readsinfo.leveldirectly rather than maintaining a side table. Thelevelfield has a default member initializer (WarningLevel::Default); this is what lets the existing 4-element aggregate initializers — both the Lua-generated ones and theDIAGNOSTIC(id, severity, name, msg)macro catalogs in compiler-core — keep compiling unchanged.Gating. In
getEffectiveMessageSeverity, after the existing pragma-tracker step, a warning whose group is not enabled is lowered toSeverity::Disable. This sits in theelseof the per-id-override check, so an explicit-W<name>/-Wno-<name>always wins over group gating.One cascading fix — keeping the enable override for grouped warnings.
overrideDiagnosticSeveritypreviously dropped any override equal to the diagnostic's nominal severity. With groups that is wrong: a warning whose group is off has an effective default of "suppressed", so an explicit-W<name>enabling it back toWarningis meaningful and must be kept, otherwise group gating would re-suppress it. The fix restricts the "remove when equal to default" shortcut toWarningLevel::Defaultdiagnostics, where the nominal and effective defaults still coincide.Input-shape check for the CLI handler. The three flags are registered as a single option with three names, so only
-Wall/-Wextra/-Wpedanticever reach theWarningLevelcase; any other spelling is a wiring bug, so the fall-through asserts viaSLANG_UNEXPECTED. Exact-match option lookup takes priority over the-W<id>prefix, so these are never confused with-W<name>.Example chosen conservatively, and no behavior change yet. Only
keyword-used-as-nameis tagged. Because thepedanticgroup is on by default in this PR, the warning still appears exactly as before (its existing test passes unchanged), and the newwarning-levels.slangtest confirms the flags are accepted and that-Wno-keyword-used-as-namestill suppresses it. Flipping pedantic off-by-default and grouping the rest of the catalog is intentionally left to follow-up, per the issue.