Skip to content

fix(lint): lint only files that we build #11247

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Aug 19, 2025
Merged

fix(lint): lint only files that we build #11247

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
36ad52d
fix(lint): lint src if ignored not configured, lint only files that w…
grandizzy Aug 8, 2025
b105232
Merge branch 'master' into issue-11234
grandizzy Aug 15, 2025
10fbdc8
LInt only built files
grandizzy Aug 15, 2025
ccea4c9
Update crates/forge/src/cmd/build.rs
grandizzy Aug 15, 2025
2123e13
Update crates/forge/src/cmd/build.rs
grandizzy Aug 15, 2025
c5bb8df
Merge remote-tracking branch 'origin' into issue-11234
grandizzy Aug 18, 2025
bb7987e
Fmt
grandizzy Aug 18, 2025
7722419
Nit: simplify lint cond
grandizzy Aug 18, 2025
3e2a4a2
Merge branch 'master' into issue-11234
0xrusowsky Aug 19, 2025
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
17 changes: 13 additions & 4 deletions crates/forge/src/cmd/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,13 @@ impl BuildArgs {

// Only run the `SolidityLinter` if there are no compilation errors
if !output.output().errors.iter().any(|e| e.is_error()) {
Copy link
Member

Choose a reason for hiding this comment

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

the condition above can be merged with this one: config.lint.lint_on_build &&

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, simplified in 7722419

self.lint(&project, &config).map_err(|err| eyre!("Lint failed: {err}"))?;
self.lint(&project, &config, &self.paths).map_err(|err| eyre!("Lint failed: {err}"))?;
Copy link
Member

Choose a reason for hiding this comment

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

can we not get the paths from the compiler output instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure we want this as will mean that we won't lint contracts that are already built, e.g. we will show lint errors only first time for forge build src/utils/BaseVault.sol but not on subsequent forge build src/utils/BaseVault.sol (same with forge build entire project). I think we want to always lint the files, wdyt?

Copy link
Member

@DaniPopes DaniPopes Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

that's true for output, however we can check the paths from artifacts_with_files().

Copy link
Member

Choose a reason for hiding this comment

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

with that we can simply pass the paths from artifacts_with_files() to lint and avoid having to input_files_iter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm, isn't this the same, artifacts_with_files() returns cached and compiled artifacts, so if we call forge build src/utils/BaseVault.sol again (and BaseVault.sol is not recompiled) then we won't know to lint BaseVault.sol only, maybe I am missing something?

Copy link
Member

Choose a reason for hiding this comment

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

we can do both

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DaniPopes artifacts_with_files returns all artifacts including artifacts from deps while input_files_iter returns only src, test and scripts that we want to lint. Could use artifacts_with_files but then have to filter out deps artifacts - IMO is cleaner by input_files_iter but happy to change if you strongly think we should use artifacts_with_files. thank you!

Copy link
Member

Choose a reason for hiding this comment

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

I see, that is fine then

}

Ok(output)
}

fn lint(&self, project: &Project, config: &Config) -> Result<()> {
fn lint(&self, project: &Project, config: &Config, files: &Option<Vec<PathBuf>>) -> Result<()> {
let format_json = shell::is_json();
if project.compiler.solc.is_some() && !shell::is_quiet() {
let linter = SolidityLinter::new(config.project_paths())
Expand Down Expand Up @@ -160,8 +160,17 @@ impl BuildArgs {
.project_paths::<SolcLanguage>()
.input_files_iter()
.filter(|p| {
skip.is_match(p)
&& !(ignored.contains(p) || ignored.contains(&curr_dir.join(p)))
// Lint only specified build files, if any.
if let Some(files) = files {
return files.iter().any(|file| &curr_dir.join(file) == p);
}
let include = if ignored.is_empty() {
// Default to source contracts only if lint ignore not configured.
p.starts_with(&config.src)
} else {
!(ignored.contains(p) || ignored.contains(&curr_dir.join(p)))
};
skip.is_match(p) && include
})
.collect::<Vec<_>>();

Expand Down
9 changes: 8 additions & 1 deletion crates/forge/src/cmd/lint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,14 @@ impl LintArgs {
config
.project_paths::<SolcLanguage>()
.input_files_iter()
.filter(|p| !(ignored.contains(p) || ignored.contains(&cwd.join(p))))
.filter(|p| {
if ignored.is_empty() {
// Default to source contracts only if lint ignore not configured.
p.starts_with(&config.src)
} else {
!(ignored.contains(p) || ignored.contains(&cwd.join(p)))
}
})
.collect()
}
paths => {
Expand Down
100 changes: 100 additions & 0 deletions crates/forge/tests/cli/lint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,24 @@ const ONLY_IMPORTS: &str = r#"
import "./ContractWithLints.sol";
"#;

const COUNTER_A: &str = r#"
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract CounterA {
uint256 public CounterA_Fail_Lint;
}
"#;

const COUNTER_B: &str = r#"
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract CounterB {
uint256 public CounterB_Fail_Lint;
}
"#;

forgetest!(can_use_config, |prj, cmd| {
prj.wipe_contracts();
prj.add_source("ContractWithLints", CONTRACT).unwrap();
Expand Down Expand Up @@ -388,6 +406,88 @@ note[unused-import]: unused imports should be removed
"#]]);
});

// <https://github.com/foundry-rs/foundry/issues/11234>
forgetest!(can_lint_src_and_build_files, |prj, cmd| {
prj.wipe_contracts();
prj.add_test("TestWithLints", COUNTER_A).unwrap();
prj.add_script("ScriptWithLints", COUNTER_A).unwrap();
// No sources, hence no lints on `forge lint` / `forge build`.
cmd.arg("lint").assert_success().stderr_eq(str![""]);
cmd.forge_fuse().arg("build").assert_success().stderr_eq(str![""]);

prj.add_source("CounterAWithLints", COUNTER_A).unwrap();
prj.add_source("CounterBWithLints", COUNTER_B).unwrap();
// Both contracts should be linted
cmd.forge_fuse().args(["lint"]).assert_success().stderr_eq(str![[r#"
note[mixed-case-variable]: mutable variables should use mixedCase
[FILE]:6:24
|
6 | uint256 public CounterA_Fail_Lint;
| ------------------
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
[FILE]:6:24
|
6 | uint256 public CounterB_Fail_Lint;
| ------------------
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable


"#]]);
// Only ContractAWithLints should be linted
cmd.forge_fuse().args(["lint", "src/CounterAWithLints.sol"]).assert_success().stderr_eq(str![
[r#"
note[mixed-case-variable]: mutable variables should use mixedCase
[FILE]:6:24
|
6 | uint256 public CounterA_Fail_Lint;
| ------------------
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable


"#]
]);

// Both contracts should be linted on build.
cmd.forge_fuse().args(["build"]).assert_success().stderr_eq(str![[r#"
note[mixed-case-variable]: mutable variables should use mixedCase
[FILE]:6:24
|
6 | uint256 public CounterA_Fail_Lint;
| ------------------
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
[FILE]:6:24
|
6 | uint256 public CounterB_Fail_Lint;
| ------------------
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable


"#]]);
// Only contract CounterBWithLints that we built should be linted.
cmd.forge_fuse().args(["build", "src/CounterBWithLints.sol"]).assert_success().stderr_eq(str![
[r#"
note[mixed-case-variable]: mutable variables should use mixedCase
[FILE]:6:24
|
6 | uint256 public CounterB_Fail_Lint;
| ------------------
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable


"#]
]);
});

// ------------------------------------------------------------------------------------------------

#[tokio::test]
Expand Down
Loading