Skip to content
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.

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

new lint: struct_added_without_non_exhaustive #1091

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Changes from all commits
Commits
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
57 changes: 57 additions & 0 deletions src/lints/struct_added_without_non_exhaustive.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
SemverQuery(
id: "struct_added_without_non_exhaustive",
human_readable_name: "pub struct added without #[non_exhaustive]",
description: "A new public struct was added without #[non_exhaustive], making it harder to evolve in the future.",
required_update: Minor,
Copy link
Owner

Choose a reason for hiding this comment

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

Since we don't have any prior "API evolution" advisory lints like this one (this is the first!), we haven't thought through what required_update should be here.

Right now, the only allowed options are Major and Minor. But neither seems quite right. Selecting Minor means that this lint won't run on major or minor releases, so new API additions like this will only be linted in patch releases. This doesn't seem quite right =/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I see Minor is not right I believe, should we put it on-hold till we figure it out

lint_level: Allow,
reference_link: Some("https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute"),
query: r#"
{
CrateDiff {
current {
item {
... on Struct {
visibility_limit @filter(op: "=", value: ["$public"]) @output
name @output
struct_type @output

# Check that it's not marked #[non_exhaustive]
attrs @filter(op: "not_contains", value: ["$non_exhaustive"])

importable_path {
path @output @tag
public_api @filter(op: "=", value: ["$true"])
}

# Verify all fields are public
field @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) {
visibility_limit @filter(op: "!=", value: ["$public"])
}

span_: span @optional {
filename @output
begin_line @output
}
}
}
}
baseline @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) {
item {
... on Struct {
importable_path {
path @filter(op: "=", value: ["%path"])
}
}
}
}
}
}"#,
arguments: {
"public": "public",
"zero": 0,
"true": true,
"non_exhaustive": "#[non_exhaustive]",
},
error_message: "A new public struct was added without #[non_exhaustive], which may make it difficult to evolve in the future",
per_result_error_template: Some("struct {{join \"::\" path}} in file {{span_filename}}:{{span_begin_line}}"),
)
1 change: 1 addition & 0 deletions src/query.rs
Original file line number Diff line number Diff line change
@@ -1208,6 +1208,7 @@ add_lints!(
struct_repr_transparent_removed,
struct_with_no_pub_fields_changed_type,
struct_with_pub_fields_changed_type,
struct_added_without_non_exhaustive,
trait_added_supertrait,
trait_allows_fewer_const_generic_params,
trait_allows_fewer_generic_type_params,
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
publish = false
name = "struct_added_without_non_exhaustive"
version = "0.1.0"
edition = "2021"

[dependencies]
50 changes: 50 additions & 0 deletions test_crates/struct_added_without_non_exhaustive/new/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Existing structs shouldn't trigger the lint
pub struct ExistingStruct {
pub field: i32,
}

// Should NOT trigger the lint - private struct
struct PrivateStruct {
pub field: i32,
}

// Should trigger the lint - new public struct with all pub fields
pub struct NewAllPubFields {
pub field1: i32,
pub field2: String,
}

// Should NOT trigger the lint - has private field
pub struct NewWithPrivateField {
pub field1: i32,
field2: String, // private field
}

// Should NOT trigger the lint - marked non_exhaustive
#[non_exhaustive]
pub struct NewWithNonExhaustive {
pub field1: i32,
pub field2: String,
}

// Should NOT trigger the lint - not public
struct NewPrivateStruct {
pub field1: i32,
pub field2: String,
}

// Should trigger the lint - unit struct
pub struct NewUnitStruct;

// Should NOT trigger the lint - private in public module
pub mod public_mod {
struct PrivateInPublicMod {
pub field: i32,
}
}

// Should trigger the lint - tuple struct with all pub fields
pub struct NewTupleStruct(pub i32, pub String);

// Should NOT trigger the lint - tuple struct with private field
pub struct NewTupleWithPrivate(pub i32, String);
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
publish = false
name = "struct_added_without_non_exhaustive"
version = "0.1.0"
edition = "2021"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Existing structs shouldn't trigger the lint
pub struct ExistingStruct {
pub field: i32,
}

struct PrivateStruct {
pub field: i32,
}
Original file line number Diff line number Diff line change
@@ -11,7 +11,6 @@ info:
env:
CARGO_TERM_COLOR: never
RUSTDOCFLAGS: "--cfg custom"
RUSTFLAGS: "--cfg custom"
RUST_BACKTRACE: "0"
---
success: false
@@ -48,7 +47,7 @@ Failed in:
Parsing cfg_conditional_compilation v0.1.0 (baseline)
Parsed [TIME] (baseline)
Checking cfg_conditional_compilation v0.1.0 -> v0.1.0 (no change)
Checked [TIME] [TOTAL] checks: [PASS] pass, 2 fail, 0 warn, 0 skip
Checked [TIME] [TOTAL] checks: [PASS] pass, 2 fail, 0 warn, 1 skip

Summary semver requires new major version: 2 major and 0 minor checks failed
Finished [TIME] cfg_conditional_compilation
Original file line number Diff line number Diff line change
@@ -26,6 +26,6 @@ exit_code: 0
Parsing cfg_conditional_compilation v0.1.0 (baseline)
Parsed [TIME] (baseline)
Checking cfg_conditional_compilation v0.1.0 -> v0.1.0 (no change)
Checked [TIME] [TOTAL] checks: [PASS] pass, 0 skip
Checked [TIME] [TOTAL] checks: [PASS] pass, 1 skip
Summary no semver update required
Finished [TIME] cfg_conditional_compilation
119 changes: 119 additions & 0 deletions test_outputs/query_execution/struct_added_without_non_exhaustive.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
---
source: src/query.rs
expression: "&query_execution_results"
---
{
"./test_crates/inherent_method_must_use_added/": [
{
"name": String("EnumToStructWithMustUseMethods"),
"path": List([
String("inherent_method_must_use_added"),
String("item_type_changed_inherent_method_must_use_added"),
String("EnumToStructWithMustUseMethods"),
]),
"span_begin_line": Uint64(8),
"span_filename": String("src/item_type_changed_inherent_method_must_use_added.rs"),
"struct_type": String("plain"),
"visibility_limit": String("public"),
},
{
"name": String("UnionToStructWithMustUseMethods"),
"path": List([
String("inherent_method_must_use_added"),
String("item_type_changed_inherent_method_must_use_added"),
String("UnionToStructWithMustUseMethods"),
]),
"span_begin_line": Uint64(161),
"span_filename": String("src/item_type_changed_inherent_method_must_use_added.rs"),
"struct_type": String("plain"),
"visibility_limit": String("public"),
},
{
"name": String("NewStructWithMustUseMethods"),
"path": List([
String("inherent_method_must_use_added"),
String("struct_inherent_method_must_use_added"),
String("NewStructWithMustUseMethods"),
]),
"span_begin_line": Uint64(65),
"span_filename": String("src/struct_inherent_method_must_use_added.rs"),
"struct_type": String("plain"),
"visibility_limit": String("public"),
},
],
"./test_crates/method_moved_to_trait_must_use_added/": [
{
"name": String("NewStructWithTraitMustUseMethods"),
"path": List([
String("method_moved_to_trait_must_use_added"),
String("struct_method_moved_to_trait_must_use_added"),
String("NewStructWithTraitMustUseMethods"),
]),
"span_begin_line": Uint64(147),
"span_filename": String("src/struct_method_moved_to_trait_must_use_added.rs"),
"struct_type": String("plain"),
"visibility_limit": String("public"),
},
],
"./test_crates/nonbreaking_item_rename/": [
{
"name": String("NewName"),
"path": List([
String("nonbreaking_item_rename"),
String("NewName"),
]),
"span_begin_line": Uint64(2),
"span_filename": String("src/lib.rs"),
"struct_type": String("unit"),
"visibility_limit": String("public"),
},
],
"./test_crates/struct_added_without_non_exhaustive/": [
{
"name": String("NewAllPubFields"),
"path": List([
String("struct_added_without_non_exhaustive"),
String("NewAllPubFields"),
]),
"span_begin_line": Uint64(12),
"span_filename": String("src/lib.rs"),
"struct_type": String("plain"),
"visibility_limit": String("public"),
},
{
"name": String("NewUnitStruct"),
"path": List([
String("struct_added_without_non_exhaustive"),
String("NewUnitStruct"),
]),
"span_begin_line": Uint64(37),
"span_filename": String("src/lib.rs"),
"struct_type": String("unit"),
"visibility_limit": String("public"),
},
{
"name": String("NewTupleStruct"),
"path": List([
String("struct_added_without_non_exhaustive"),
String("NewTupleStruct"),
]),
"span_begin_line": Uint64(47),
"span_filename": String("src/lib.rs"),
"struct_type": String("tuple"),
"visibility_limit": String("public"),
},
],
"./test_crates/switch_away_from_reexport_as_underscore/": [
{
"name": String("Struct"),
"path": List([
String("switch_away_from_reexport_as_underscore"),
String("Struct"),
]),
"span_begin_line": Uint64(4),
"span_filename": String("src/lib.rs"),
"struct_type": String("plain"),
"visibility_limit": String("public"),
},
],
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
---
source: src/snapshot_tests.rs
expression: result
snapshot_kind: text
---
success: false
--- stdout ---
@@ -26,7 +25,7 @@ Failed in:
Parsing semver_trick_self_referential v0.1.0 (baseline)
Parsed [TIME] (baseline)
Checking semver_trick_self_referential v0.1.0 -> v0.1.1 (minor change)
Checked [TIME] [TOTAL] checks: [PASS] pass, 1 fail, 0 warn, 7 skip
Checked [TIME] [TOTAL] checks: [PASS] pass, 1 fail, 0 warn, 8 skip

Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [TIME] semver_trick_self_referential
Original file line number Diff line number Diff line change
@@ -54,7 +54,7 @@ note: the following command can be used to reproduce the compilation error:
Parsing no-error v0.1.0 (baseline)
Parsed [TIME] (baseline)
Checking no-error v0.1.0 -> v0.1.0 (no change)
Checked [TIME] [TOTAL] checks: [PASS] pass, 1 fail, 0 warn, 0 skip
Checked [TIME] [TOTAL] checks: [PASS] pass, 1 fail, 0 warn, 1 skip

Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [TIME] no-error
Original file line number Diff line number Diff line change
@@ -54,7 +54,7 @@ note: the following command can be used to reproduce the compilation error:
Parsing no-error v0.1.0 (baseline)
Parsed [TIME] (baseline)
Checking no-error v0.1.0 -> v0.1.0 (no change)
Checked [TIME] [TOTAL] checks: [PASS] pass, 1 fail, 0 warn, 0 skip
Checked [TIME] [TOTAL] checks: [PASS] pass, 1 fail, 0 warn, 1 skip

Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [TIME] no-error
Original file line number Diff line number Diff line change
@@ -25,7 +25,7 @@ Failed in:
Parsing a v0.1.0 (baseline)
Parsed [TIME] (baseline)
Checking a v0.1.0 -> v0.1.0 (no change)
Checked [TIME] [TOTAL] checks: [PASS] pass, 1 fail, 0 warn, 0 skip
Checked [TIME] [TOTAL] checks: [PASS] pass, 1 fail, 0 warn, 1 skip

Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [TIME] a