Skip to content

Linter: add rule to remove empty then else #1819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Linter: add rule to remove empty then else #1819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/extension/alterschema/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ sourcemeta_library(NAMESPACE sourcemeta PROJECT core NAME alterschema
linter/if_without_then_else.h
linter/max_contains_without_contains.h
linter/min_contains_without_contains.h
linter/then_else_empty.h
linter/then_without_if.h)

if(SOURCEMETA_CORE_INSTALL)
Expand Down
2 changes: 2 additions & 0 deletions src/extension/alterschema/alterschema.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ static auto every_item_is_boolean(const T &container) -> bool {
#include "linter/pattern_properties_default.h"
#include "linter/properties_default.h"
#include "linter/single_type_array.h"
#include "linter/then_else_empty.h"
#include "linter/then_without_if.h"
#include "linter/unevaluated_items_default.h"
#include "linter/unevaluated_properties_default.h"
Expand All @@ -101,6 +102,7 @@ auto add(SchemaTransformer &bundle, const AlterSchemaMode mode)
bundle.add<IfWithoutThenElse>();
bundle.add<MaxContainsWithoutContains>();
bundle.add<MinContainsWithoutContains>();
bundle.add<ThenElseEmpty>();
bundle.add<ThenWithoutIf>();
bundle.add<DependenciesPropertyTautology>();
bundle.add<DependentRequiredTautology>();
Expand Down
36 changes: 36 additions & 0 deletions src/extension/alterschema/linter/then_else_empty.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
class ThenElseEmpty final : public SchemaTransformRule {
public:
ThenElseEmpty()
: SchemaTransformRule{
"then_else_empty",
"`then` or `else` with an empty schema does not restrict "
"validation and is likely ineffective"} {};
Copy link
Member

@jviotti jviotti Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

"likely ineffective" is wrong. It is not likely. It does nothing, in all cases.

For consistency with other rules, can you make the message: "Setting the then or else keywords to the empty schema does not add any further constraint"

Copy link
Member

Choose a reason for hiding this comment

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

Actually, just to make the error simpler by just talking about a single keyword, can you split this rule into two? One of then and one for else similar to how we catch then without if and else without if?


[[nodiscard]] auto
condition(const JSON &schema, const JSON &, const Vocabularies &vocabularies,
const SchemaFrame &, const SchemaFrame::Location &,
const SchemaWalker &, const SchemaResolver &) const
-> SchemaTransformRule::Result override {
return contains_any(
vocabularies,
{"https://json-schema.org/draft/2020-12/vocab/applicator",
"https://json-schema.org/draft/2019-09/vocab/applicator",
"http://json-schema.org/draft-07/schema#"}) &&
schema.is_object() && schema.defines("if") &&
((schema.defines("then") && schema.at("then").is_object() &&
schema.at("then").empty()) ||
Copy link
Member

Choose a reason for hiding this comment

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

As in the other PR, remember that schemas can be either objects or boolean. You are missing checking if then or else are set to true, which is also equivalent to the empty schema (and tests for those)

(schema.defines("else") && schema.at("else").is_object() &&
schema.at("else").empty()));
}

auto transform(JSON &schema) const -> void override {
if (schema.defines("then") && schema.at("then").is_object() &&
schema.at("then").empty()) {
schema.erase("then");
}
if (schema.defines("else") && schema.at("else").is_object() &&
schema.at("else").empty()) {
schema.erase("else");
}
}
};
157 changes: 157 additions & 0 deletions test/alterschema/alterschema_lint_2019_09_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1680,3 +1680,160 @@ TEST(AlterSchema_lint_2019_09, unnecessary_allof_ref_wrapper_5) {

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2019_09, then_else_empty_1) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema"
})JSON");

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2019_09, then_else_empty_2) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"else": {}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema"
})JSON");

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2019_09, then_else_empty_3) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {},
"else": {}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema"
})JSON");

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2019_09, then_else_empty_4) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {
"type": "string"
},
"else": {}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {
"type": "string"
}
})JSON");

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2019_09, then_else_empty_5) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {
"type": "string"
},
"else": {
"type": "number"
}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {
"type": "string"
},
"else": {
"type": "number"
}
})JSON");

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2019_09, then_else_empty_6) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema",
"then": {}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema"
})JSON");

EXPECT_EQ(document, expected);
}
157 changes: 157 additions & 0 deletions test/alterschema/alterschema_lint_2020_12_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1842,3 +1842,160 @@ TEST(AlterSchema_lint_2020_12, unnecessary_allof_ref_wrapper_5) {

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2020_12, then_else_empty_1) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema"
})JSON");

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2020_12, then_else_empty_2) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"else": {}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema"
})JSON");

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2020_12, then_else_empty_3) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {},
"else": {}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema"
})JSON");

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2020_12, then_else_empty_4) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {
"type": "string"
},
"else": {}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {
"type": "string"
}
})JSON");

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2020_12, then_else_empty_5) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {
"type": "string"
},
"else": {
"type": "number"
}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema",
"if": {
"properties": {
"flag": {
"const": true
}
}
},
"then": {
"type": "string"
},
"else": {
"type": "number"
}
})JSON");

EXPECT_EQ(document, expected);
}

TEST(AlterSchema_lint_2020_12, then_else_empty_6) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema",
"then": {}
})JSON");

LINT_AND_FIX_FOR_READABILITY(document);

const sourcemeta::core::JSON expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2020-12/schema"
})JSON");

EXPECT_EQ(document, expected);
}
Loading