feat(linter): add forbid_empty_enum rule#2287
feat(linter): add forbid_empty_enum rule#2287Vaibhav701161 wants to merge 6 commits intosourcemeta:mainfrom
Conversation
8006bfe to
43d131d
Compare
|
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
|
|
Good point ! If the tests confirm that an empty I'll report back once the test suite PR is up. |
|
Great. I guess given the other PR is approved, we can attempt to auto-fix to |
|
Thanks! @jviotti , that makes sense. |
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>
There was a problem hiding this comment.
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.
23840b3 to
ec6066e
Compare
|
@jviotti I’ve pushed the updates to make the rule auto-fixable and added the corresponding tests. 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) { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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>
| EXPECT_TRUE(result.first); | |
| EXPECT_FALSE(result.first); |
|
@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. |
Summary
This PR introduces a new lint rule:
forbid_empty_enum.The rule detects schemas declaring an empty
enumarray.{ "enum": [] }An empty
enummakes 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:
enumexistsThe diagnostic points to the
enumkeyword 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:
src/extension/alterschema/linter/forbid_empty_enum.halterschema.ccSOURCESlist in the alterschema CMake configurationThe rule checks:
enumkeyword is definedenumis an arrayIf 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:
enumenumcontains valuesenumis absentproperties)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