-
Notifications
You must be signed in to change notification settings - Fork 428
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
Linter: Warn when number primitive is annotated with #[ink(topic)]
#1837
Conversation
The problem with suppressions is that `rustc` saves the state of the last visited node and checks its attributes to decide if the lint warning should be suppressed there (see: [LateContext::last_node_with_lint_attrs](https://github.com/rust-lang/rust/blob/c44324a4fe8e96f9d6473255df6c3a481caca76f/compiler/rustc_lint/src/context.rs#L1105)). This commit adds a call to the utility function from clippy that checks if the lint is suppressed for the field definition node, not for the last node (which is `Topics` impl).
Rework the lint after use-ink#1827
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks very good. We just need to decide whether to merge it as a minor patch of the ink! 4, or make it compatible with Events 2.0 and merge it in ink! 5.
I personally think we should make it work with Event 2.0 as ink! 5.0 might be released soon, and we will still have to add support for the new event definition.
@SkymanOne Thanks for the review. I commited all the suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to run the tests locally I got a bunch of compilation errors related to clippy_utils
.
The requested changes are what helped me to resolve the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update ref of clippy_utils
to 1d334696587ac22b3a9e651e7ac684ac9e0697b2
and bump dylint_linting
& dylint_testing
to 2.1.12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to set channel to nightly-2023-08-10
in rust-toolchain.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I updated clippy_utils
version to 1d334696587ac22b3a9e651e7ac684ac9e0697b2
and set channel to nightly-2023-07-14
in rust-toolchain.toml
. nightly-2023-07-14
is required, since this version is used in rust-clippy
at commit 1d334696587ac22b3a9e651e7ac684ac9e0697b2
.
How should we choose the clippy
version for linting
? Should we always take the latest version from the clippy
master
branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by clippy
here. If you are referring to clippy_utils
, I've been following a template in dylint repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By clippy
I mean the rust-clippy repository, which contains the clippy_utils
crate. In this repo they set the toolchain version to nightly-2023-07-14
for all the crates. So we need to set the same version in our toolchain file to build clippy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK we only have rust-toolchain.toml
for the lining crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Feel free to merge whenever you are ready
Closes #1436