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

Add ink! linting MVP stage #431

Merged
merged 45 commits into from
Feb 23, 2022
Merged

Add ink! linting MVP stage #431

merged 45 commits into from
Feb 23, 2022

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Feb 15, 2022

Closes #281.

This is an MVP, there are a couple shortcomings for which I want to create follow-up issues:

  • As it is now, we unfortunately have to require users to install dylint and dylint-driver manually. We should look into making those cargo dependencies instead. However, this is not straight forward. Especially for the dylint-driver (which is used as a linker) I'm not sure if this is even possible.

  • I haven't yet managed to get rid of some driver build overhead, this happens every time the linting is applied:

    [1/2] Checking ink! lints                                                     
    Building driver for toolchain `nightly-2021-11-04-x86_64-unknown-linux-gnu`        
    
  • We should render and publish a proper website displaying info about the linting rules. Similar to https://rust-lang.github.io/rust-clippy/master/.

  • We should recognize if the call to ink_lang::utils::initialize_contract is nested within the constructor. So if it's not called directly in the constructor, but instead in a sub-function.

How it works:

  • ink_linting/ contains the ink! linting driver (i.e. the linting rules).
  • build.rs builds these rules into a *.so/*.dll, this file is included via include_bytes! into the cargo-contract executable.

Try it out:

git checkout cmichi-implemnt-dylint-mvp
RUST_LOG=info cargo run -- contract check --manifest-path ../ink/examples/flipper/Cargo.toml --verbose

@xgreenx
Copy link
Collaborator

xgreenx commented Feb 15, 2022

We should recognize if the call to ink_lang::codegen::initialize_contract is nested within the constructor. So if it's not called directly in the constructor, but instead in a sub-function.

Maybe better to require the user to always implement SpreadAllocate and we will use ink_lang::codegen::initialize_contract in the codegen?

If the contract implements SpreadAllocate we can create an instance of the contract. After we can pass that instance into the constructor and the user will do all the necessary stuff.

Example of the idea in the playground.

impl Contract {
    #[ink(constrcutor)]
    fn new(mut self/* we will require `self` here*/, total_supply: u32) -> Self {
        self.total_supply = total_supply;
        self
    }
}

@cmichi cmichi force-pushed the cmichi-implemnt-dylint-mvp branch 2 times, most recently from de13be5 to 899d3a7 Compare February 16, 2022 19:52
@cmichi
Copy link
Collaborator Author

cmichi commented Feb 17, 2022

@xgreenx Thanks for your comment! So the thing is that we don't want to change the ink! constructor function signature, we would really like to stay as close to Rust syntax as possible.

Putting the initialize_contract call into the codegen would result in a changed function signature though. This has implications for e.g. the off-chain testing, so there some special API be needed so that it's still possible to e.g. write Rust unit tests for the contract.

Our thinking right now is that we will likely find a better way going forward and by requiring the initalize_contract in the meantime those contracts would be forward compatible. If we change the function signature now though, then those contracts can't be easily used with newer versions of ink! if we change the function signature back.

@cmichi cmichi force-pushed the cmichi-implemnt-dylint-mvp branch 5 times, most recently from ad3962b to 27451a6 Compare February 17, 2022 06:04
@cmichi cmichi marked this pull request as ready for review February 17, 2022 06:24
@cmichi cmichi requested a review from athei February 17, 2022 06:25
@cmichi cmichi force-pushed the cmichi-implemnt-dylint-mvp branch 5 times, most recently from c1a772a to 228aa04 Compare February 17, 2022 07:02
@xgreenx
Copy link
Collaborator

xgreenx commented Feb 17, 2022

Thanks for the response. I see you are right. I forgot about off-chain unit tests that the problem still exists for them.

Our thinking right now is that we will likely find a better way going forward and by requiring the initalize_contract in the meantime those contracts would be forward compatible. If we change the function signature now though, then those contracts can't be easily used with newer versions of ink! if we change the function signature back.

Yesterday I tried to implement your idea regarding the new structure of the contract. Here is results. Maybe you can check it in your free time=)

.github/workflows/macos.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
.github/workflows/macos.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
Co-authored-by: Sergejs Kostjucenko <85877331+sergejparity@users.noreply.github.com>
src/cmd/build.rs Show resolved Hide resolved
src/cmd/build.rs Outdated Show resolved Hide resolved
src/cmd/build.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

);
zip_dir(&template_dir, &template_dst_file, CompressionMethod::Stored).map(|_| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a fan of mapping into an io side effect, but now I am just nitpicking

@athei
Copy link
Contributor

athei commented Feb 23, 2022

I am wondering: We are writing our custom link but don't even run the stock clippy lints. Can we do that at the same time as we are running dylint so we don't have multiple passes?

@cmichi cmichi merged commit 836acbf into master Feb 23, 2022
@cmichi cmichi deleted the cmichi-implemnt-dylint-mvp branch February 23, 2022 17:14
@cmichi
Copy link
Collaborator Author

cmichi commented Feb 23, 2022

We are writing our custom link but don't even run the stock clippy lints. Can we do that at the same time as we are running dylint so we don't have multiple passes?

So dylint doesn't support the stock clippy lints out of the box, it would be an additional invocation of cargo clippy. We could do that, but I'm on the fence since we then would become way more opinionated.

@xgreenx
Copy link
Collaborator

xgreenx commented Feb 25, 2022

Is that possible to use CARGO_TARGET_DIR with dylint to not build it each time?
Also maybe make dylint configurable? For example, I don't want to build it during integration tests=)

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.

Evaluate custom ink! lints using Dylint
5 participants