feat(linter): add const_and_enum_conflict rule#2290
Conversation
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/alterschema/alterschema_lint_2019_09_test.cc">
<violation number="1" location="test/alterschema/alterschema_lint_2019_09_test.cc:4512">
P2: New const_and_enum_conflict tests do not assert both diagnostic keyword locations, so regressions in secondary-location reporting would go undetected.</violation>
</file>
<file name="test/alterschema/alterschema_lint_2020_12_test.cc">
<violation number="1" location="test/alterschema/alterschema_lint_2020_12_test.cc:9682">
P2: New `const_and_enum_conflict` tests do not assert the diagnostic’s dual keyword locations, so regressions in secondary-location reporting would go undetected.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
074cd14 to
be2a9a0
Compare
Signed-off-by: AcE <kintan0108@gmail.com>
be2a9a0 to
d8c5e5d
Compare
|
Very interesting and well implemented. I think what we can do it split this rule into a few variations, as some are auto-fixable. For example:
Let's maybe add and test both variants? |
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/alterschema/alterschema_lint_draft7_test.cc">
<violation number="1" location="test/alterschema/alterschema_lint_draft7_test.cc:3658">
P2: Regression: const/enum lint tests no longer assert diagnostic trace details (rule id/message/path), weakening contract coverage.</violation>
</file>
<file name="test/alterschema/alterschema_lint_draft6_test.cc">
<violation number="1" location="test/alterschema/alterschema_lint_draft6_test.cc:2933">
P2: Tests for the const+enum conflict path were weakened by removing assertions on lint trace metadata (rule id/message/locations), reducing coverage of user-visible diagnostics.</violation>
<violation number="2" location="test/alterschema/alterschema_lint_draft6_test.cc:2945">
P2: New `LINT_AND_FIX` tests assert success (`EXPECT_TRUE(result.first)`) despite expecting schema mutations, conflicting with this suite’s established result contract for applied fixes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ba4e095 to
e504562
Compare
|
@jviotti I have updated the PR based on your feedback I also noticed your recent review on PR 2287, so I applied those same testing conventions to this PR (dropping trace checks, strict Summary of updates:
|
e504562 to
e10eee6
Compare
Signed-off-by: AcE <kintan0108@gmail.com>
e10eee6 to
15b0605
Compare
There was a problem hiding this comment.
@AcEKaycgR Actually just a last minor thing: can you move this rule to the common directory? I think its fine to run both here and for the canonicalizer
There was a problem hiding this comment.
Understood i will do that now
Signed-off-by: AcE <kintan0108@gmail.com>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/extension/alterschema/alterschema.cc">
<violation number="1" location="src/extension/alterschema/alterschema.cc:181">
P2: `ConstInEnum` was moved to always-on/common registration, so a mutating transform now runs outside linter mode, while the intended linter-specific `const_and_enum_conflict` rule is not present/registered.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@jviotti Done Moved const_in_enum.h to the common/ directory and registered it for both the linter and canonicalizer. |
Summary
This PR introduces a new lint rule:
const_and_enum_conflict.The rule detects schemas that declare both
constandenumkeywords simultaneously.{ "const": 1, "enum": [1, 2, 3] }Using both
constandenumtogether is redundant and contradictory.constis equivalent to a single-valueenum, so having both is always an authoring mistake and should be reported to the user.The rule emits a diagnostic when:
constis definedenumis definedThe diagnostic points to both the
constandenumkeyword locations.This rule is non auto-fixable because the correct resolution depends on the schema author's intent (whether to keep
constorenum).Implementation
The rule follows the existing alterschema linter architecture:
src/extension/alterschema/linter/const_and_enum_conflict.halterschema.ccThe rule checks:
constkeyword is definedenumkeyword is definedNote: The rule only applies from Draft 6 onward, as
constwas introduced in Draft 6.If these conditions are met the rule returns:
Tests
Tests were added across supported dialects using the existing lint testing utilities.
The following scenarios are covered:
constandenumare presentconstis presentenumis presentproperties)All tests follow the patterns used by existing lint rule tests.
Related Work
This rule addresses one of the pending lint rules listed in:
Refs: #1975
@jviotti Could you please review this pull request when you have a moment?
If the approach looks good, I'd be happy to continue implementing additional linting rules listed in #1975.
cc @Karan-Palan
Note: PR description and test structure follow the same format as 2287 for consistency.