Make vertex-shader-missing-sv-position pedantic and off by default#11893
Open
expipiplus1 wants to merge 14 commits into
Open
Make vertex-shader-missing-sv-position pedantic and off by default#11893expipiplus1 wants to merge 14 commits into
expipiplus1 wants to merge 14 commits into
Conversation
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.
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.
Fixes shader-slang#11884. E38052 fires on any vertex shader whose output lacks SV_Position, but a vertex shader may legitimately omit it when its output feeds a geometry/tessellation/mesh stage rather than the rasterizer, which the compiler cannot tell apart at VS-compile time. Move it into the pedantic warning group and flip the group defaults so pedantic is off by default (opt in with -Wpedantic) and extra is on by default. Reclassify keyword-used-as-name (E20103) from pedantic to extra so it keeps firing by default. Builds on shader-slang#11812.
expipiplus1
requested review from
bmillsNV and
Copilot
and removed request for
a team
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds opt-in warning groups for diagnostics, with public warning-level types and compiler options, sink-side gating and override handling, CLI/Lua wiring, updated diagnostic metadata, and coverage/docs for the new warning flags. ChangesOpt-in warning group system
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts Slang’s diagnostic warning-group defaults to be clang/gcc-like (independent -Wall/-Wextra/-Wpedantic, with extra enabled and pedantic disabled by default) and reclassifies selected warnings so noisy/false-positive diagnostics become opt-in.
Changes:
- Flip default enabled warning groups so
extrais on by default andpedanticis off by default, while preserving per-id override precedence. - Retag diagnostics: move
vertex-shader-missing-sv-position(E38052) topedantic, andkeyword-used-as-name(E20103) toextra; update the E38052 message to remove stale suppression advice. - Update tests and docs to reflect the new warning-group behavior and flags.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/slang-unit-test/unit-test-diagnostic-warning-level.cpp | Adds unit tests for DiagnosticSink warning-level gating and override interactions (needs updates to match new defaults). |
| tests/diagnostics/warning-levels.slang | End-to-end test for the extra warning group behavior via E20103. |
| tests/diagnostics/vertex-missing-sv-position.slang | Updates regression coverage so E38052 is off by default and enabled via -Wpedantic / per-id -W.... |
| source/slang/slang-rich-diagnostics.h.lua | Adds mapping from Lua warning-level strings to C++ WarningLevel enum values. |
| source/slang/slang-rich-diagnostics.cpp | Emits DiagnosticInfo entries including the new level field. |
| source/slang/slang-options.cpp | Registers and parses -Wall/-Wextra/-Wpedantic as additive warning-group enables. |
| source/slang/slang-diagnostics.lua | Binds warning-level sentinels and retags E20103/E38052 with updated messaging/comments. |
| source/slang/slang-diagnostics-helpers.lua | Introduces and plumbs all/extra/pedantic sentinels through diagnostic processing. |
| source/slang/slang-compiler-options.cpp | Applies the repeatable WarningLevel compiler option to the sink with validation. |
| source/compiler-core/slang-diagnostic-sink.h | Defines WarningLevel, adds the DiagnosticInfo::level field, and implements group enable/query with default extra enabled. |
| source/compiler-core/slang-diagnostic-sink.cpp | Implements warning-group gating in getEffectiveMessageSeverity and preserves meaningful per-id enables for grouped warnings. |
| include/slang.h | Adds public SlangWarningLevel and exposes CompilerOptionName::WarningLevel. |
| docs/user-guide/08-compiling.md | Documents the new WarningLevel compiler option and the default group behavior. |
| docs/command-line-slangc-reference.md | Adds CLI reference entries for -Wall/-Wextra/-Wpedantic. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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: b5259967-454e-44d3-828a-43ddefe474f0
📒 Files selected for processing (14)
docs/command-line-slangc-reference.mddocs/user-guide/08-compiling.mdinclude/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/vertex-missing-sv-position.slangtests/diagnostics/warning-levels.slangtools/slang-unit-test/unit-test-diagnostic-warning-level.cpp
Update unit-test-diagnostic-warning-level.cpp for the new group defaults (Extra on, Pedantic/All off): default-gating, additive-enable and per-id-override tests now use off-by-default groups where they need one and an on-by-default group (Extra) to show -Wno winning over an enabled group. Refresh the DiagnosticInfo::level doc and fix a stale test-file reference.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add negative cases pinning the independent-groups invariant: the pedantic vertex-shader-missing-sv-position warning must stay silent when only the other groups (-Wall/-Wextra) are enabled.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Document the fourth SlangWarningLevel enumerator (DEFAULT) as the always-on group that is a no-op for the WarningLevel option.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The auto-generated 38052 catalog regression compiled a VS without SV_Position and asserted W38052 with no flags; since E38052 is now a pedantic (off-by-default) warning, add -Wpedantic so the nightly catalog test keeps firing it.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Keep only the functional -Wpedantic addition in the generated catalog regression; drop the hand-written prose so a future regeneration has less to reconcile.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add a comma after "if it feeds the rasterizer directly" so the message no longer runs "directly the rasterizer" together; update the two test annotations that quote the secondary span.
Contributor
There was a problem hiding this comment.
Verdict: 🟡 Has issues — 4 gap(s)
Retags E38052 (vertex-shader-missing-sv-position) as pedantic (off by default) and E20103 (keyword-used-as-name) as extra (on by default), flipping the default enabled-groups bitmask so Pedantic requires opt-in via -Wpedantic. Findings are two hand-edits to auto-generated files and two coverage gaps for the added public API / child-sink propagation paths.
Changes Overview
Warning-group infrastructure (include/slang.h, source/compiler-core/slang-diagnostic-sink.{h,cpp}, source/slang/slang-compiler-options.cpp, source/slang/slang-options.cpp)
- Adds
SlangWarningLevelenum andCompilerOptionName::WarningLevel = 155to the public API, and aWarningLevelfield onDiagnosticInfowith aDefaultdefault so aggregate-initialized diagnostics still compile. DiagnosticSinkgains auint32_t m_enabledWarningLevelsbitmask (initialized withExtraon,Pedantic/Alloff), enable/is-enabled/get/set accessors with abit < 32guard against shift UB, gating ingetEffectiveMessageSeverityafter per-id overrides, and parent-copy propagation in the child constructor.overrideDiagnosticSeverityno longer removes aseverity==overrideSeverityoverride when the diagnostic's group is notDefault, so-W<name>on a grouped warning force-enables it.- CLI adds
-Wall/-Wextra/-WpedanticunderOptionKind::WarningLevel;applySettingsToDiagnosticSinkfiltersintValueat the API boundary.
Diagnostic retagging (source/slang/slang-diagnostics.lua, source/slang/slang-diagnostics-helpers.lua, source/slang/slang-rich-diagnostics.{cpp,h.lua})
- Adds
all/extra/pedanticpositional sentinels to the Lua diagnostic DSL;add_diagnosticrecordsdiag.levelandprocess_diagnosticspropagates it;getWarningLevelEnummaps toWarningLevel::*. - E38052 gets
pedantictag and its message drops the stale-warnings-disable 38052suffix; E20103 getsextratag.
Tests (tests/diagnostics/vertex-missing-sv-position.slang, tests/diagnostics/warning-levels.slang, tools/slang-unit-test/unit-test-diagnostic-warning-level.cpp)
- E38052 test rewritten to cover off-by-default,
-Wall/-Wextranon-effect,-Wpedantic, per-id-Wno-, and per-id-W<name>cases. - New
warning-levels.slangcovers theextragroup via E20103 (default/-Wextra/-Wall/-Wpedantic/-Wno-). - Unit tests cover default gating, additive enable, per-id override precedence, warnings-as-errors interaction, and out-of-range enum safety.
Docs (include/slang.h, docs/user-guide/08-compiling.md, docs/command-line-slangc-reference.md, docs/generated/tests/design/cross-cutting/diagnostics-catalog/38052-vertex-shader-missing-sv-position.slang)
- New
WarningLevelrow in theCompilerOptionValuetable; new-Wall,-Wextra,-Wpedanticsection in the CLI reference; the generated catalog test for E38052 adds-Wpedanticto its directive.
Findings (4 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | docs/generated/tests/.../38052-vertex-shader-missing-sv-position.slang:16 |
Hand-edit to a file whose header says "Do not edit by hand"; regenerate via docs/generated/tests/_meta/regenerate.py. |
| 🟡 Gap | docs/command-line-slangc-reference.md:261-268 |
Hand-edit to a file generated by slangc -help-style markdown -h; regenerate after the CLI option is registered. |
| 🟡 Gap | source/slang/slang-compiler-options.cpp:434-449 |
No test exercises the public CompilerOptionName::WarningLevel path through applySettingsToDiagnosticSink. |
| 🟡 Gap | source/compiler-core/slang-diagnostic-sink.h:425 |
No test covers propagation of m_enabledWarningLevels to child sinks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11884. Stacked on #11812 — review/merge that first; the diff here will show #11812's commits until it lands.
Motivation
E38052 (
vertex-shader-missing-sv-position) warns whenever a vertex shader has outputs but none carrySV_Position. That is a false positive whenever the vertex shader legitimately omits the position because its output feeds a geometry/tessellation/mesh stage that supplies it — which the compiler cannot distinguish from a real missing-position bug at VS-compile time. The reporter hit it on aGS_INPUT MainVs(...)vertex shader feeding a geometry shader.Proposed solution
Use the warning-group mechanism added in #11812:
extrais on by default andpedanticis off by default (clang/gcc-style, independent groups). Add diagnostic warning levels (-Wall/-Wextra/-Wpedantic) #11812 shippedpedanticon-by-default only as a staging step; this is the promised follow-up that turns it off.vertex-shader-missing-sv-position(E38052) aspedantic. It now fires only under-Wpedantic(or-Wvertex-shader-missing-sv-position), so the default build is quiet.keyword-used-as-name(E20103) frompedantictoextraso it keeps firing by default (it still points at a real, if benign, footgun). This makes E38052 the canonicalpedanticexample — a warning that genuinely should be off by default.-warnings-disable 38052suggestion from the E38052 message; the warning is opt-in now.Change summary
source/compiler-core/slang-diagnostic-sink.hExtraon,Pedanticoff.source/slang/slang-diagnostics.luapedantic(and reword its message); retag E20103extra; bind theextrasentinel.tests/diagnostics/warning-levels.slangextragroup (E20103): on by default, unaffected by-Wall/-Wpedantic, suppressible by-Wno.tests/diagnostics/vertex-missing-sv-position.slangpedanticregression test: off by default, on under-Wpedanticor per-id-W<name>, re-suppressed by-Wno.docs/user-guide/08-compiling.mdWarningLevelnote for the new defaults.Process report
Positive and negative cases are covered by exhaustive diag buffers: a buffer with annotations asserts the warning IS emitted (and nothing else is), and a buffer with no annotations asserts NOTHING is emitted. E38052:
diag=OFFwith no flags (silent),diag=ONunder-Wpedanticand under-Wvertex-shader-missing-sv-position,diag=OFFunder-Wpedantic -Wno-.... E20103:diag=WARNby default /-Wextra/-Wall/-Wpedantic(independent groups leave it on),diag=OFFunder-Wno-....The change sits at the right layer: the diagnostic itself is correct (a VS with no SV_Position may indeed drop positions), it is only its default visibility that was wrong for the VS→GS case, which is exactly what the warning-group gating controls.