Skip to content

feat(linter): add const_and_enum_conflict rule#2290

Merged
jviotti merged 3 commits intosourcemeta:mainfrom
AcEKaycgR:feat/linter-const-and-enum-conflict
Mar 17, 2026
Merged

feat(linter): add const_and_enum_conflict rule#2290
jviotti merged 3 commits intosourcemeta:mainfrom
AcEKaycgR:feat/linter-const-and-enum-conflict

Conversation

@AcEKaycgR
Copy link
Contributor

Summary

This PR introduces a new lint rule: const_and_enum_conflict.

The rule detects schemas that declare both const and enum keywords simultaneously.

{
  "const": 1,
  "enum": [1, 2, 3]
}

Using both const and enum together is redundant and contradictory. const is equivalent to a single-value enum, so having both is always an authoring mistake and should be reported to the user.

The rule emits a diagnostic when:

  • const is defined
  • enum is defined
  • both exist on the same schema object

The diagnostic points to both the const and enum keyword locations.

This rule is non auto-fixable because the correct resolution depends on the schema author's intent (whether to keep const or enum).

Implementation

The rule follows the existing alterschema linter architecture:

  • implemented as a header-only rule in src/extension/alterschema/linter/const_and_enum_conflict.h
  • registered in alterschema.cc
  • added to the SOURCES list in the alterschema CMake configuration

The rule checks:

  • schema node is an object
  • const keyword is defined
  • enum keyword is defined

Note: The rule only applies from Draft 6 onward, as const was introduced in Draft 6.

If these conditions are met the rule returns:

APPLIES_TO_KEYWORDS("const", "enum")

Tests

Tests were added across supported dialects using the existing lint testing utilities.

The following scenarios are covered:

  • Rule fires when both const and enum are present
  • Rule does not fire when only const is present
  • Rule does not fire when only enum is present
  • Rule fires inside nested subschemas (e.g. inside properties)

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.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-ai with guidance or docs links (including llms.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.

@AcEKaycgR AcEKaycgR force-pushed the feat/linter-const-and-enum-conflict branch 2 times, most recently from 074cd14 to be2a9a0 Compare March 10, 2026 07:21
Signed-off-by: AcE <kintan0108@gmail.com>
@AcEKaycgR AcEKaycgR force-pushed the feat/linter-const-and-enum-conflict branch from be2a9a0 to d8c5e5d Compare March 10, 2026 07:31
@jviotti
Copy link
Member

jviotti commented Mar 13, 2026

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:

  • If both enum and const are there but the value in const is ALSO inside enum, then it is safe to just delete enum in the transform. This can be some sort of const_in_enum rule?
  • If both enum and const are there but the value in const is NOT inside enum, the whole subschema is not satisfiable. Maybe it makes sense to just transform the entire subschema to false (to highlight the impossibility?) though maybe better for a UI/UX point of view on the linter to report it and not auto-fix it like you did here

Let's maybe add and test both variants?

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@AcEKaycgR AcEKaycgR force-pushed the feat/linter-const-and-enum-conflict branch from ba4e095 to e504562 Compare March 14, 2026 03:44
@AcEKaycgR
Copy link
Contributor Author

@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 LINT_AND_FIX formatting, and adding edge case tests to ensure the auto-fixer safely preserves siblings like $id and custom annotations).

Summary of updates:

  • Split the rule: Replaced the generic rule with const_in_enum (auto-fixes by safely removing the enum keyword) and const_not_in_enum (leaves as non-fixable to report the contradiction).
  • Updated test structure: Aligned the test formats with the latest repository conventions (dropped trace checks, used strict LINT_AND_FIX ordering).
  • Added edge cases: Added tests to explicitly prove the auto-fixer preserves siblings like $id and custom annotations without wiping out the surrounding object.

@AcEKaycgR AcEKaycgR force-pushed the feat/linter-const-and-enum-conflict branch from e504562 to e10eee6 Compare March 17, 2026 14:53
@AcEKaycgR AcEKaycgR force-pushed the feat/linter-const-and-enum-conflict branch from e10eee6 to 15b0605 Compare March 17, 2026 15:52
Copy link
Member

@jviotti jviotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood i will do that now

Signed-off-by: AcE <kintan0108@gmail.com>
Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@AcEKaycgR
Copy link
Contributor Author

@jviotti Done Moved const_in_enum.h to the common/ directory and registered it for both the linter and canonicalizer.

@jviotti jviotti merged commit 9116b0f into sourcemeta:main Mar 17, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants