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

[Linter] Constant naming #16819

Merged
merged 15 commits into from
May 14, 2024
Merged

Conversation

tx-tomcat
Copy link
Contributor

Description

Constant Naming enforces a naming convention for constants in Move programs, requiring them to follow an ALL_CAPS_SNAKE_CASE or PascalCase/UpperCamelCase format. This lint checks each constant's name within a module against this convention.

Run the linter

cargo test move_check_testsuit

Testing

File testing: constant_naming.move

Copy link

vercel bot commented Mar 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2024 5:30pm
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2024 5:30pm
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2024 5:30pm
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview May 14, 2024 5:30pm
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview May 14, 2024 5:30pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview May 14, 2024 5:30pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview May 14, 2024 5:30pm

@stefan-mysten stefan-mysten requested review from a team March 25, 2024 23:41
@tx-tomcat
Copy link
Contributor Author

I have resolved all of these comment above

Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

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

Let's work through these comments. And also be sure to fix the failing CI tests (you might need to rename some constants somewhere)

Comment on lines 28 to 29
LinterDiagCategory::ConstantNaming as u8,
LINTER_DEFAULT_DIAG_CODE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's build up some categories here. So we should decide on the category for linting and then make the code one specific to constant naming

Choose a reason for hiding this comment

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

refactored

Severity::Warning,
LinterDiagCategory::ConstantNaming as u8,
LINTER_DEFAULT_DIAG_CODE,
"Constant name should be in all caps, snake case, pascal case or upper camel case.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Constant name should be in all caps, snake case, pascal case or upper camel case.",
"non-conventional constant name",

The message on the error code should be relatively short

Copy link
Contributor

Choose a reason for hiding this comment

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

"constant should follow naming convention" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that a lot more, thanks!

Choose a reason for hiding this comment

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

refactored

.iter()
.for_each(|(loc, name, _constant)| {
if !is_valid_name(name.as_str()) {
let uid_msg = format!("'{}' should be named using UPPER_CASE_WITH_UNDERSCORES or PascalCase/UpperCamelCase",name.as_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting looks off here. We probably need to split the line

Suggested change
let uid_msg = format!("'{}' should be named using UPPER_CASE_WITH_UNDERSCORES or PascalCase/UpperCamelCase",name.as_str());
let uid_msg = format!(
"'{name}' should be named using UPPER_CASE_WITH_UNDERSCORES \
or PascalCase/UpperCamelCase",
);

Choose a reason for hiding this comment

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

refactored

}

impl TypingVisitorContext for Context<'_> {
fn visit(&mut self, program: &mut T::Program_) {
Copy link
Contributor

@tnowacki tnowacki Apr 9, 2024

Choose a reason for hiding this comment

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

This should be overriding visit_constant_custom

Choose a reason for hiding this comment

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

refactored

@@ -83,6 +85,7 @@ pub enum LinterDiagCategory {
FreezeWrapped,
CollectionEquality,
PublicRandom,
ConstantNaming,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a separate category/code system for the linters outside of the sui_mode linters

Choose a reason for hiding this comment

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

refactored

const MIN_THRESHOLD: u64 = 10;
const MIN_U64: u64 = 10;

// // Incorrectly named constants
Copy link
Contributor

Choose a reason for hiding this comment

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

This says incorrectly named, but they shouldn't trigger a warning? I find that a bit confusing

Choose a reason for hiding this comment

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

refactored

Comment on lines 2 to 5
// Correctly named constants
const MAX_LIMIT: u64 = 1000;
const MIN_THRESHOLD: u64 = 10;
const MIN_U64: u64 = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we split this into two separate test files. One with all the valid cases and another with all of the invalid ones?

Choose a reason for hiding this comment

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

yes.We could. refactored

@@ -0,0 +1,20 @@
module 0x42::M {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could make the file something like external-crates/move/crates/move-compiler/tests/linter/constant_naming.move

The change here is changing the dir custom_rules to linter

Choose a reason for hiding this comment

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

refactored

public_random::PublicRandomVisitor.visitor(),
]
}
LintLevel::Default | LintLevel::All => vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, revert spurious change

Comment on lines 82 to 94
fn is_valid_name(name: &str) -> bool {
if name
.chars()
.all(|c| c.is_uppercase() || c == '_' || c.is_numeric())
{
return true;
}
// Check for PascalCase/UpperCamelCase
// The string must start with an uppercase letter, and only contain alphanumeric characters,
// with every new word starting with an uppercase letter.
let mut chars = name.chars();
chars.next().unwrap().is_uppercase() && chars.all(|c| c.is_alphanumeric())
}
Copy link
Contributor

@cgswords cgswords Apr 9, 2024

Choose a reason for hiding this comment

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

Nit: you can do this in a single pass over the string:

fn is_valid_name(name: &str) -> bool {
    let mut chars = name.chars();
    let Some(start) = chars.next() else { return false /* ice? */ };
    if !start.is_uppercase() { return false }

    let mut all_uppers = true;
    let mut has_underscore = false;
    while let Some(char) = chars.next() {
        if char.is_lowercase() {
            all_uppers = false;
        } else if char == '_' {
            has_underscore = true;
        } else if !char.is_alphanumeric() {
            return false; // bail if it's not alphanumeric ?
        }

        // We have an underscore but we have non-uppercase letters
        if has_underscore && !all_uppers {
            return false;
        }
    }
    true
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if we kept this in the slightly slower implementation that is easier to understand.

Choose a reason for hiding this comment

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

refactored

@cgswords
Copy link
Contributor

cgswords commented Apr 9, 2024

This needs a test for the allow as well

.iter()
.for_each(|(loc, name, _constant)| {
if !is_valid_name(name.as_str()) {
let uid_msg = format!("'{}' should be named using UPPER_CASE_WITH_UNDERSCORES or PascalCase/UpperCamelCase",name.as_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

But we might want to switch this to the following for brevity (and maybe clarity)

format!("'{name}' should be ALL_CAPS. Or for error constants, use PascalCase")

Choose a reason for hiding this comment

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

refactored

Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

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

99% the way there!
Just a few small things:

  • A few small nits (see comments)
  • I believe the changes to sui_mode/liters/mod.rs can be reverted
  • Let's make sure we have a test for the warning suppression, both on the module and constant
  • I've kicked off CI and it looks like there are some errors

// SPDX-License-Identifier: Apache-2.0

//! `ConstantNamingVisitor` enforces a naming convention for constants in Move programs,
//! requiring them to follow an ALL_CAPS_SNAKE_CASE format. This lint checks each constant's name
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//! requiring them to follow an ALL_CAPS_SNAKE_CASE format. This lint checks each constant's name
//! requiring them to follow an ALL_CAPS_SNAKE_CASE or PascalCase format. This lint checks each constant's name

Choose a reason for hiding this comment

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

refactored

Comment on lines 25 to 26
pub const CONSTANT_NAMING_DIAG_CODE: u8 = 2;
pub const LINTER_DEFAULT_DIAG_CODE: u8 = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub const CONSTANT_NAMING_DIAG_CODE: u8 = 2;
pub const LINTER_DEFAULT_DIAG_CODE: u8 = 1;
pub const CONSTANT_NAMING_DIAG_CODE: u8 = 1;

Choose a reason for hiding this comment

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

refactored

@@ -73,6 +73,8 @@ pub const RANDOM_MOD_NAME: &str = "random";
pub const RANDOM_STRUCT_NAME: &str = "Random";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can revert all changes to this file?

const Maxcount: u64 = 500; // Should not trigger a warning
const MinValue: u64 = 1; // Should not trigger a warning

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, no trailing newline

Choose a reason for hiding this comment

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

refactored

const JSON_Max_Size: u64 = 1048576;
const VALUE_MAX: u64 = 200;
const numItems: u64 = 30;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, missing trailing newline

Choose a reason for hiding this comment

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

refactored

Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

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

Just one more round should do it :)
The things to fix:

  • Tests should test one thing, so remove the unrelated things from incorrect_constant_naming.move
  • Let's generalize suppressed_incorrect_constant_naming.move for all future lints
  • We should not emit lints for things that already errors. In this case, that means no lint for constants where the name is invalid. (this should also include restricted names like Self)

Comment on lines 13 to 15
// Should not trigger a warning
#[allow(lint(constant_naming))]
const numItems: u64 = 30;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Should not trigger a warning
#[allow(lint(constant_naming))]
const numItems: u64 = 30;

Let's remove this. Tests should test one thing, with the suppression, it is testing two: the errors and the suppression of the errors

Choose a reason for hiding this comment

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

removed

@@ -0,0 +1,15 @@
#[allow(lint(constant_naming))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Since there is going to be a lot of these lints coming, let's have one test that has all of the future lint allows in them.

So something like
move-compiler/tests/linter/suppressed_lints.move

And we can cut this test down to one case per lint, so

module 0x42::M {
    const Another_BadName: u64 = 42; // Should trigger a warning
}

Choose a reason for hiding this comment

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

refactored

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Let's remove this file as it is now superseded by the other ones

module 0x42::M {
// Incorrectly named constants
const Another_BadName: u64 = 42; // Should trigger a warning
const YetAnotherName: u64 = 777; // Should not trigger a warning
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, remove any valid names from this test

Choose a reason for hiding this comment

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

refactored

Comment on lines 6 to 7
const minimumValue: u64 = 10;
const some_important_number: u64 = 55;
Copy link
Contributor

Choose a reason for hiding this comment

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

I should have caught this a lot earlier, sorry.
These constants have invalid names. There is no need to lint against constants where the name is an error.

Choose a reason for hiding this comment

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

refactored


#[allow(lint(constant_naming))]
const Another_BadName: u64 = 42; // Should trigger a warning
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
}
}

Choose a reason for hiding this comment

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

refactored

@@ -0,0 +1,15 @@
#[allow(lint(constant_naming))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Let's remove this file as it is now superseded by the other ones

CONSTANT_NAMING_DIAG_CODE,
Some(CONSTANT_NAMING_FILTER_NAME),
)],
)
}

pub fn linter_visitors(level: LintLevel) -> Vec<Visitor> {
match level {
LintLevel::None => vec![],
LintLevel::Default | LintLevel::All => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We only want this on for All

Suggested change
LintLevel::Default | LintLevel::All => {
LintLevel::Default => vec![],
LintLevel::All => {

Choose a reason for hiding this comment

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

refactored

@tnowacki tnowacki mentioned this pull request May 7, 2024
7 tasks
tnowacki added a commit that referenced this pull request May 8, 2024
## Description 

- Fixing an issue exposed in #16819. Since Sui lints use the `lint`
prefix for suppression, we need to make sure all category/code combos
are distinct.
- Added new lint categories, with a specific one for Sui lints

## Test plan 

- Updated tests 

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

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

See the comment about the additional files being brought in last commit.
Additionally, there are some test/CI failures that need to be addressed.

After those two things are fixed, this should be good to go in!

@@ -0,0 +1,109 @@
//! Detect potential overflow scenarios where the number of bits being shifted exceeds the bit width of
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like linter+tests was accidentally brought into this PR last commit. Let's remove it so we can focus on it in its own PR

Choose a reason for hiding this comment

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

removed

Copy link

vercel bot commented May 14, 2024

@dzungdinh94 is attempting to deploy a commit to the Mysten Labs Team on Vercel.

A member of the Team first needs to authorize it.

datatest_stable::harness!(move_check_testsuite, "tests/", r".*\.move$");
datatest_stable::harness!(move_check_testsuite, "tests/linter", r".*\.move$");
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you having trouble running your tests? It should already be picked up via the LINTER_DIR check

Choose a reason for hiding this comment

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

i accidently changes this files. restored

Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

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

Thanks for all the iterations on this!

@tnowacki tnowacki merged commit 9dcb3ef into MystenLabs:main May 14, 2024
42 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants