Skip to content

feat(linter): add forbid_empty_enum rule#2287

Open
Vaibhav701161 wants to merge 6 commits intosourcemeta:mainfrom
Vaibhav701161:feat/linter-forbid-empty-enum
Open

feat(linter): add forbid_empty_enum rule#2287
Vaibhav701161 wants to merge 6 commits intosourcemeta:mainfrom
Vaibhav701161:feat/linter-forbid-empty-enum

Conversation

@Vaibhav701161
Copy link

Summary

This PR introduces a new lint rule: forbid_empty_enum.

The rule detects schemas declaring an empty enum array.

{
  "enum": []
}

An empty enum makes the schema unsatisfiable because no instance can ever match it.
This is usually an authoring mistake and should be reported to the user.

The rule emits a diagnostic when:

  • enum exists
  • the value is an array
  • the array is empty

The diagnostic points to the enum keyword location.

This rule is non auto-fixable because the correct resolution depends on the schema author's intent.


Implementation

The rule follows the existing alterschema linter architecture:

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

The rule checks:

  • schema node is an object
  • enum keyword is defined
  • enum is an array
  • array length is zero

If these conditions are met the rule returns:

APPLIES_TO_KEYWORDS("enum")

Tests

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

The following scenarios are covered:

  1. Rule fires for an empty enum
  2. Rule does not fire when enum contains values
  3. Rule does not fire when enum is absent
  4. 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

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.

No issues found across 8 files

@Vaibhav701161
Copy link
Author

@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.

@Vaibhav701161 Vaibhav701161 force-pushed the feat/linter-forbid-empty-enum branch from 8006bfe to 43d131d Compare March 7, 2026 15:36
@jviotti
Copy link
Member

jviotti commented Mar 9, 2026

Ah, cool. Thanks for raising this up! I'm now thinking whether this can be auto-fixable or not. Here is the thing:

Can you send a PR to the tests checking what happens with an empty enum and cc me? I guess it means validate nothing, but sounds like something to discuss with the group. Depending on that outcome, maybe we can make it auto-fixable? For example:

  • If it does nothing, we can just auto-fix by removing the enum
  • If it validates nothing, you can turn the entire current subschema to false

@Vaibhav701161
Copy link
Author

Good point !
I'll open a PR in the JSON Schema Test Suite adding test cases for an empty enum so we can clarify the intended behaviour, and cc you there.

If the tests confirm that an empty enum makes the schema unsatisfiable, we could potentially make this rule auto-fixable by replacing the subschema with false. If the intended behaviour is that it has no effect, then the auto-fix could simply remove the enum keyword.

I'll report back once the test suite PR is up.

@jviotti
Copy link
Member

jviotti commented Mar 12, 2026

Great. I guess given the other PR is approved, we can attempt to auto-fix to false

@Vaibhav701161
Copy link
Author

Thanks! @jviotti , that makes sense.
I’ll update the forbid_empty_enum rule to make it auto-fixable and transform the subschema to false, and add the corresponding tests for the transformation.

Signed-off-by: Vaibhav mittal <vaibhavmittal929@gmail.com>
Signed-off-by: Vaibhav mittal <vaibhavmittal929@gmail.com>
…draft7

Signed-off-by: Vaibhav mittal <vaibhavmittal929@gmail.com>
Signed-off-by: Vaibhav mittal <vaibhavmittal929@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.

4 issues found across 15 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/linter/forbid_empty_enum.h">

<violation number="1" location="src/extension/alterschema/linter/forbid_empty_enum.h:34">
P2: For Draft 1–4 schemas this transform rewrites a schema object to the boolean literal `false`, but those drafts require schemas to be JSON objects (boolean schemas only became valid in draft‑06). This can turn a valid Draft‑1–4 schema into an invalid schema document for those dialects.</violation>
</file>

<file name="test/alterschema/alterschema_lint_2020_12_test.cc">

<violation number="1" location="test/alterschema/alterschema_lint_2020_12_test.cc:9866">
P2: Autofix replaces any schema object with an empty `enum` by `false`, even when the object has sibling keywords (e.g., annotations like `x-note`). This drops annotations/identifiers such as `$id`/`$anchor` and is not semantics-preserving for schema documents. The new tests assert this unsafe rewrite (e.g., `items` with `x-note` becomes `false`).</violation>
</file>

<file name="src/core/yaml/stringify.h">

<violation number="1" location="src/core/yaml/stringify.h:216">
P2: Floating‑point YAML serialization uses the stream’s default precision/locale, which can round doubles and emit non‑portable numeric strings. This risks lossy or locale‑dependent YAML output.</violation>
</file>

<file name="test/alterschema/alterschema_lint_draft6_test.cc">

<violation number="1" location="test/alterschema/alterschema_lint_draft6_test.cc:3006">
P3: `forbid_empty_enum_4` expects `result.first` to be true even though a lint trace is asserted, which is inconsistent with the rest of the `LINT_AND_FIX` tests and the harness convention (false when lint findings occur).</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@Vaibhav701161 Vaibhav701161 force-pushed the feat/linter-forbid-empty-enum branch from 23840b3 to ec6066e Compare March 13, 2026 11:39
@Vaibhav701161
Copy link
Author

@jviotti I’ve pushed the updates to make the rule auto-fixable and added the corresponding tests.
When you have a moment, could you please review the changes?

Also, Cubic left a few automated comments , please let me know if any of them are relevant require changes on my side.

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2019_09, forbid_empty_enum_1) {
Copy link
Member

Choose a reason for hiding this comment

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

Now that the rule is always fixable, let's always exercise it with LINT_AND_FIX. Also closely follow the conventions of the other tests. We have expected after the check for result.first, and we don't usually check the traces anymore.

return APPLIES_TO_KEYWORDS("enum");
}

auto transform(JSON &schema, const Result &) const -> void override {
Copy link
Member

@jviotti jviotti Mar 13, 2026

Choose a reason for hiding this comment

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

Can you add a LINT_AND_FIX test where you have a $ref pointing directly at the subschema with the empty enum and also one pointing to something INSIDE the subschema with empty enum (maybe by having a $defs sibling to the empty enum)?

I suspect the second will break at the moment.

Also what if you have an empty enum on a subschema that has its own nested $id? By turning the entire thing to false, we would end up getting rid of the identifier, which can break a reference to the subschema by URI.

Can you try to encode as many of these edge cases as you can possibly think of? These are the tricky cases that show up in practice and end up in incorrect linter behaviour or crashes

Given all the above, it might be easier to add not: {} instead of turning the entire thing to false. And you can always update the condition to apply for Draft 4 and later (which will address https://github.com/sourcemeta/core/pull/2287/changes#r2931230208) and only apply of not is not there already (a fine compromise for now?)

Signed-off-by: Vaibhav mittal <vaibhavmittal929@gmail.com>
…eference edge cases

Signed-off-by: Vaibhav mittal <vaibhavmittal929@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 7 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_draft6_test.cc">

<violation number="1" location="test/alterschema/alterschema_lint_draft6_test.cc:2944">
P2: LINT_AND_FIX result semantics in this test suite expect result.first to be false when a rule applies and mutates the document. The new forbid_empty_enum_* tests assert true even when the document is changed, which is inconsistent and likely incorrect.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.


LINT_AND_FIX(document, result, traces);

EXPECT_TRUE(result.first);
Copy link

@cubic-dev-ai cubic-dev-ai bot Mar 13, 2026

Choose a reason for hiding this comment

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

P2: LINT_AND_FIX result semantics in this test suite expect result.first to be false when a rule applies and mutates the document. The new forbid_empty_enum_* tests assert true even when the document is changed, which is inconsistent and likely incorrect.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/alterschema/alterschema_lint_draft6_test.cc, line 2944:

<comment>LINT_AND_FIX result semantics in this test suite expect result.first to be false when a rule applies and mutates the document. The new forbid_empty_enum_* tests assert true even when the document is changed, which is inconsistent and likely incorrect.</comment>

<file context>
@@ -2931,52 +2931,75 @@ TEST(AlterSchema_lint_draft6, empty_object_as_true_1) {
-                    "An empty `enum` validates nothing and is equivalent to "
-                    "`false`",
-                    true);
+  EXPECT_TRUE(result.first);
+
+  const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
</file context>
Suggested change
EXPECT_TRUE(result.first);
EXPECT_FALSE(result.first);
Fix with Cubic

@Vaibhav701161
Copy link
Author

@jviotti , I've updated the tests to always use LINT_AND_FIX and edge cases for $ref (direct and nested via $defs), subschemas with $id, and annotation preservation.

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