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

API: Include the lint crate in the lint identifier #288

Merged
merged 2 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion .github/workflows/gh-pages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ on:
# Match only on specific tags. We don't want this workflow to be invoked when
# we put sliding `v{major}` and `v{major}.{minor}` tags on the same commit.
tags: ['v[0-9]+.[0-9]+.[0-9]+*']
workflow_dispatch:
workflow_dispatch:
xFrednet marked this conversation as resolved.
Show resolved Hide resolved

env:
CARGO_TERM_COLOR: always
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ The [v0.4.0 milestone] contains a list of planned changes.

[v0.4.0 milestone]: https://github.com/rust-marker/marker/milestone/4
[#278]: https://github.com/rust-marker/marker/pull/278
[#288]: https://github.com/rust-marker/marker/pull/288

### Breaking Changes
- [#278]: Moved the `span()` method from the inherent impl to `HasSpan` trait for `ClosureParam`, `FnParam`, `StructFieldPat`.
- [#278]: The trait `TyData` no longer has own `span()` method, but instead requires `HasSpan` as a supertrait.
- [#278]: All public methods that took `&Span` as a parameter now take `impl HasSpan`. This is a minor breaking change, as `&Span` implements `HasSpan`, but if you relied on type inference based on the function parameter type, then making a method generic may break your code.
- [#288]: Lint identifiers use the lint crate name as a new infix, to prevent name collisions across lint crates.

## [0.3.0] - 2023-10-05

Expand Down
33 changes: 25 additions & 8 deletions marker_api/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,16 @@ use crate::common::{Level, MacroReport};
#[repr(C)]
#[derive(Debug, PartialEq, Eq, Hash)]
pub struct Lint {
/// A string identifier for the lint.
/// The string identifier of this lint.
///
/// This identifies the lint in attributes and in command-line arguments.
/// In those contexts it is always lowercase. This allows
/// [`declare_lint`](crate::declare_lint) macro invocations to follow the
/// convention of upper-case statics without repeating the name.
/// Identifiers use underscores, to connect multiple words, e.g., "unused_imports".
/// Lint names in console arguments use dashes instead of underscores. These are
/// automatically converted to underscores internally.
///
/// The name is written with underscores, e.g., "unused_imports".
/// On the command line, underscores become dashes.
/// Identifiers in Marker are split into three parts:
/// 1. The `marker` prefix, specifying that the lint comes from Marker
/// 2. The lint crate name, as an infix
/// 3. The name of the lint, as the postfix
///
/// See <https://rustc-dev-guide.rust-lang.org/diagnostics.html#lint-naming>
/// for naming guidelines.
Expand Down Expand Up @@ -118,7 +119,23 @@ macro_rules! declare_lint {
) => {
$(#[doc = $doc])+
pub static $NAME: &$crate::Lint = &$crate::Lint {
name: concat!("marker::", stringify!($NAME)),
// The `CARGO_CRATE_NAME` environment value is set by Cargo during compilation.
// The name has already been normalized to use underscores, instead of dashes,
// which is ideal for Marker.
//
// The name is taken from the `Cargo.toml` file of the lint crate, even if the
// dependency has been renamed. This is fine for now, since Marker doesn't expose
// any interface for the users to rename the lint crates. We might need to add our
// own environment value later, if we want to support lint crate renaming.
//
// Example, how the package could be renamed in the `Cargo.toml` of Marker's dummy
// crate for lint crate fetching:
// ```toml
// [dependencies]
// ducks = { version = "0.3", package = "marker_lints" }
// ```
// The environment value would still have the value `marker_lints`
name: concat!("marker::", std::env!("CARGO_CRATE_NAME"), "::", stringify!($NAME)),
default_level: $crate::common::Level::$LEVEL,
explanation: concat!($($doc, '\n',)*),
report_in_macro: $REPORT_IN_MACRO,
Expand Down
2 changes: 1 addition & 1 deletion marker_lints/tests/ui/diag_msg_uppercase_start.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ warning: this message starts with an uppercase character
21 | cx.emit_lint(DUMMY, expr, "X <-- starting with upper case");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(marker::diag_msg_uppercase_start)]` on by default
= note: `#[warn(marker::marker_lints::diag_msg_uppercase_start)]` on by default

warning: this message starts with an uppercase character
--> $DIR/diag_msg_uppercase_start.rs:22:31
Expand Down
2 changes: 1 addition & 1 deletion marker_lints/tests/ui/not_using_has_span_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
marker,
feature(register_tool),
register_tool(marker),
warn(marker::not_using_has_span_trait)
warn(marker::marker_lints::not_using_has_span_trait)
)]

use marker_api::prelude::*;
Expand Down
4 changes: 2 additions & 2 deletions marker_lints/tests/ui/not_using_has_span_trait.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ warning: use impl HasSpan instead of Span for more flexibility
note: the lint level is defined here
--> $DIR/not_using_has_span_trait.rs:5:10
|
5 | warn(marker::not_using_has_span_trait)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5 | warn(marker::marker_lints::not_using_has_span_trait)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: use impl HasSpan instead of Span for more flexibility
--> $DIR/not_using_has_span_trait.rs:11:19
Expand Down
7 changes: 6 additions & 1 deletion marker_lints/tests/uitest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@ use marker_uitest::ui_test::*;
use std::env;

fn main() -> color_eyre::Result<()> {
let mut config = marker_uitest::simple_ui_test_config!("tests/ui", "../target")?;
let mut config: Config = marker_uitest::simple_ui_test_config!("tests/ui", "../target")?;

config.dependencies_crate_manifest_path = Some("./Cargo.toml".into());

// `marker_api::declare_lint` uses the `CARGO_CRATE_NAME` environment value.
// Setting it here once, makes sure that it'll be available, during the
// compilation of all ui test files.
std::env::set_var("CARGO_CRATE_NAME", "marker_lints_uitests");
Copy link
Contributor

@Veetaha Veetaha Oct 11, 2023

Choose a reason for hiding this comment

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

This looks like a painful papercut. I don't fully understand how it works. Why is this marker_lints_uitests and not just marker_lints? The name marker_lints_uitests isn't mentioned anywhere else in this PR. I also don't see this code setup for marker_uilints.

Could this be avoided? Otherwise, could the workaround be encapsulated inside of marker_uitest::simple_ui_test_config?

Copy link
Member Author

Choose a reason for hiding this comment

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

ui_test doesn't use Cargo and instead calls the rustc driver directly. This is for a few reasons, as Cargo would need a workspace, Cargo.toml file and would also be a bit slower for only compiling a single file. However, this also means that any environment values usually set by Cargo (starting with CARGO_*) are not set when the uitest files are compiled.

This should really only be noticed, if someone writes lints for the marker_api::declare_lint! macro. That's why only the uitest of marker_lints required this environment value to be set. So I don't really see it as a paper cut.


config.filter(r"\\/", "/");
config.filter(r"\\\\", "/");

Expand Down
6 changes: 3 additions & 3 deletions marker_uilints/tests/ui/cfg_attr_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
#![cfg_attr(marker, register_tool(marker))]

mod allow_with_simple_attr {
#[cfg_attr(marker, allow(marker::item_with_test_name))]
#[cfg_attr(marker, allow(marker::marker_uilints::item_with_test_name))]
fn find_me_fn() {}
}

mod allow_with_crate_check_attr {
#[cfg_attr(marker = "marker_uilints", allow(marker::item_with_test_name))]
#[cfg_attr(marker = "marker_uilints", allow(marker::marker_uilints::item_with_test_name))]
fn find_me_fn() {}
}

Expand All @@ -17,6 +17,6 @@ mod lint_with_unloaded_crate_attr {
}

mod unknown_lint_allow {
#[cfg_attr(marker, allow(marker::some_unknown_lint_that_doesnt_exist))]
#[cfg_attr(marker, allow(marker::hey::some_unknown_lint_that_doesnt_exist))]
fn foo() {}
}
8 changes: 4 additions & 4 deletions marker_uilints/tests/ui/cfg_attr_check.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
warning: unknown lint: `marker::some_unknown_lint_that_doesnt_exist`
warning: unknown lint: `marker::hey::some_unknown_lint_that_doesnt_exist`
--> $DIR/cfg_attr_check.rs:20:30
|
20 | #[cfg_attr(marker, allow(marker::some_unknown_lint_that_doesnt_exist))]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
20 | #[cfg_attr(marker, allow(marker::hey::some_unknown_lint_that_doesnt_exist))]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unknown_lints)]` on by default

Expand All @@ -12,7 +12,7 @@ warning: found a `fn` item with a test name
16 | fn find_me_fn() {}
| ^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(marker::item_with_test_name)]` on by default
= note: `#[warn(marker::marker_uilints::item_with_test_name)]` on by default

warning: 2 warnings emitted

2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/context/test_ast_map.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ warning: testing `AstMap::variant`
| ^^^^^^^^^^^^^^^^
|
= note: `AstMap::variant()` --> None
= note: `#[warn(marker::test_ast_map)]` on by default
= note: `#[warn(marker::marker_uilints::test_ast_map)]` on by default

warning: testing `AstMap::variant`
--> $DIR/test_ast_map.rs:12:26
Expand Down
2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/expr/print_async_block.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ warning: print test
capture_kind: Move,
},
)
= note: `#[warn(marker::test_lint)]` on by default
= note: `#[warn(marker::marker_uilints::test_lint)]` on by default

warning: 1 warning emitted

2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/expr/print_await_expr.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ warning: print test
),
},
)
= note: `#[warn(marker::test_lint)]` on by default
= note: `#[warn(marker::marker_uilints::test_lint)]` on by default

warning: print test
--> $DIR/print_await_expr.rs:9:5
Expand Down
2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/expr/print_closure_expr.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ warning: print test
body_id: BodyId(..),
},
)
= note: `#[warn(marker::test_lint)]` on by default
= note: `#[warn(marker::marker_uilints::test_lint)]` on by default

warning: print test
--> $DIR/print_closure_expr.rs:7:5
Expand Down
2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/expr/sugar/README.md
Original file line number Diff line number Diff line change
@@ -1 +1 @@
This folder contains tests, to check that expressions are resugared correctly and their spans also match up.
This folder contains tests, to check that expressions are resugared correctly and their spans also match up.
2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/expr/sugar/for_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ fn main() {
let range = 1..10;
let mut total = 0;

#[warn(marker::print_every_expr)]
#[warn(marker::marker_uilints::print_every_expr)]
for i in range {
total += i;
}
Expand Down
4 changes: 2 additions & 2 deletions marker_uilints/tests/ui/expr/sugar/for_loop.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ warning: expr
note: the lint level is defined here
--> $DIR/for_loop.rs:8:12
|
8 | #[warn(marker::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
8 | #[warn(marker::marker_uilints::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: expr
--> $DIR/for_loop.rs:9:14
Expand Down
8 changes: 4 additions & 4 deletions marker_uilints/tests/ui/expr/sugar/ranges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
#![register_tool(marker)]

fn main() {
#[warn(marker::print_every_expr)]
#[warn(marker::marker_uilints::print_every_expr)]
let _ = 1..2;
#[warn(marker::print_every_expr)]
#[warn(marker::marker_uilints::print_every_expr)]
let _ = ..2;
#[warn(marker::print_every_expr)]
#[warn(marker::marker_uilints::print_every_expr)]
let _ = 1..;
#[warn(marker::print_every_expr)]
#[warn(marker::marker_uilints::print_every_expr)]
let _ = ..;
}
16 changes: 8 additions & 8 deletions marker_uilints/tests/ui/expr/sugar/ranges.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ warning: expr
note: the lint level is defined here
--> $DIR/ranges.rs:5:12
|
5 | #[warn(marker::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
5 | #[warn(marker::marker_uilints::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: expr
--> $DIR/ranges.rs:6:13
Expand Down Expand Up @@ -61,8 +61,8 @@ warning: expr
note: the lint level is defined here
--> $DIR/ranges.rs:7:12
|
7 | #[warn(marker::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
7 | #[warn(marker::marker_uilints::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: expr
--> $DIR/ranges.rs:8:15
Expand Down Expand Up @@ -94,8 +94,8 @@ warning: expr
note: the lint level is defined here
--> $DIR/ranges.rs:9:12
|
9 | #[warn(marker::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
9 | #[warn(marker::marker_uilints::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: expr
--> $DIR/ranges.rs:10:13
Expand Down Expand Up @@ -127,8 +127,8 @@ warning: expr
note: the lint level is defined here
--> $DIR/ranges.rs:11:12
|
11 | #[warn(marker::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
11 | #[warn(marker::marker_uilints::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: 8 warnings emitted

4 changes: 2 additions & 2 deletions marker_uilints/tests/ui/expr/sugar/try_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@
#![register_tool(marker)]

fn try_something() -> Option<u32> {
#[warn(marker::print_every_expr)]
#[warn(marker::marker_uilints::print_every_expr)]
Some(21)?;
None
}

fn kanske_option() -> Result<i32, u32> {
let x: Result<i32, u32> = Err(1);
#[warn(marker::print_every_expr)]
#[warn(marker::marker_uilints::print_every_expr)]
x?;
Ok(42)
}
Expand Down
8 changes: 4 additions & 4 deletions marker_uilints/tests/ui/expr/sugar/try_expr.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ warning: expr
note: the lint level is defined here
--> $DIR/try_expr.rs:5:12
|
5 | #[warn(marker::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
5 | #[warn(marker::marker_uilints::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: expr
--> $DIR/try_expr.rs:6:5
Expand Down Expand Up @@ -61,8 +61,8 @@ warning: expr
note: the lint level is defined here
--> $DIR/try_expr.rs:12:12
|
12 | #[warn(marker::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
12 | #[warn(marker::marker_uilints::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: expr
--> $DIR/try_expr.rs:13:5
Expand Down
2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/expr/sugar/while_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
fn main() {
let mut something = Some(12);

#[warn(marker::print_every_expr)]
#[warn(marker::marker_uilints::print_every_expr)]
while let Some(_) = something {
something = None;
}
Expand Down
4 changes: 2 additions & 2 deletions marker_uilints/tests/ui/expr/sugar/while_loop.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ warning: expr
note: the lint level is defined here
--> $DIR/while_loop.rs:7:12
|
7 | #[warn(marker::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
7 | #[warn(marker::marker_uilints::print_every_expr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: expr
--> $DIR/while_loop.rs:8:11
Expand Down
2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/find_item.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ help: a spanned help
|
8 | static FIND_ITEM: u32 = 4;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: `#[warn(marker::test_lint)]` on by default
= note: `#[warn(marker::marker_uilints::test_lint)]` on by default

warning: 1 warning emitted

2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/foo_items.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ warning: found a `module` item with a test name
19 | | }
| |_^
|
= note: `#[warn(marker::item_with_test_name)]` on by default
= note: `#[warn(marker::marker_uilints::item_with_test_name)]` on by default

warning: found a `const` item with a test name
--> $DIR/foo_items.rs:2:5
Expand Down
2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/item/print_async_fn.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ warning: printing item with body
},
),
}
= note: `#[warn(marker::test_lint)]` on by default
= note: `#[warn(marker::marker_uilints::test_lint)]` on by default

warning: printing item with body
--> $DIR/print_async_fn.rs:9:10
Expand Down
2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/item/print_fn_item.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ warning: printing item
),
},
)
= note: `#[warn(marker::test_lint)]` on by default
= note: `#[warn(marker::marker_uilints::test_lint)]` on by default

warning: printing item
--> $DIR/print_fn_item.rs:3:21
Expand Down
2 changes: 1 addition & 1 deletion marker_uilints/tests/ui/item_id_resolution.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ warning: check type resolution
= note: Is this a "std::string::String" -> false
= note: Is this a "std::option::Option" -> false
= note: Is this a "crate::TestType" -> false
= note: `#[warn(marker::test_lint)]` on by default
= note: `#[warn(marker::marker_uilints::test_lint)]` on by default

warning: check type resolution
--> $DIR/item_id_resolution.rs:6:5
Expand Down
4 changes: 2 additions & 2 deletions marker_uilints/tests/ui/lint_level_attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@

const FIND_ME_DEFAULT: i32 = 0;

#[allow(marker::item_with_test_name)]
#[allow(marker::marker_uilints::item_with_test_name)]
const FIND_ME_ALLOW: i32 = 0;

#[deny(marker::item_with_test_name)]
#[deny(marker::marker_uilints::item_with_test_name)]
const FIND_ME_DENY: i32 = 0;
Loading