feat(lint): refactor LinterConfig and add 3 new linting rules #12581
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
milosdjurica
requested review from
DaniPopes,
grandizzy,
mattsse,
onbjerg and
zerosnacks
as code owners
Contributor
|
@milosdjurica first of all thanks for the PR and the initiative, we love new contributions to forge's linter! i think the idea overall makes sense, however imo we can improve it.
lmk if this seems reasonable and if u have any doubts regarding impl |
Contributor
Author
Sounds great, I like it! Will look to implement it ASAP :) |
…i_contract_file_exceptions
Feat/linting interface naming
milosdjurica
changed the title
milosdjurica
changed the title
Contributor
|
@milosdjurica lmk when the PR is ready for re-review! |
Contributor
Author
|
Hi @0xrusowsky , it should be good now 👍🏻 |
Contributor
|
great, tysm! will review it today at some point |
0xrusowsky
requested changes
Nov 30, 2025
Contributor
0xrusowsky
left a comment
0xrusowsky
left a comment
There was a problem hiding this comment.
just left some nits for code clarity and succinctness, but otherwise the changes LGTM!
also, sorry for the late review
Comment on lines
67
to
83
| fn check_item_contract(&mut self, ctx: &LintContext, contract: &'ast ast::ItemContract<'ast>) { | ||
| if !ctx.is_lint_enabled(INTERFACE_NAMING.id()) { | ||
| return; | ||
| } | ||
|
|
||
| // Only check interfaces | ||
| if contract.kind != ast::ContractKind::Interface { | ||
| return; | ||
| } | ||
|
|
||
| // Check if interface name starts with 'I' | ||
| let name = contract.name.as_str(); | ||
| if !name.starts_with('I') { | ||
| ctx.emit(&INTERFACE_NAMING, contract.name.span); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
nit, can we simplify with a single let chain? no need for early exit as there is really no branching
fn check_item_contract(&mut self, ctx: &LintContext, contract: &'ast ast::ItemContract<'ast>) {
if ctx.is_lint_enabled(INTERFACE_NAMING.id())
&& let ast::ContractKind::Interface = contract.kind
&& !contract.name.as_str().starts_with('I')
{
ctx.emit(&INTERFACE_NAMING, contract.name.span);
}
}
Comment on lines
23
to
65
| fn check_full_source_unit( | ||
| &mut self, | ||
| ctx: &LintContext<'ast, '_>, | ||
| unit: &'ast ast::SourceUnit<'ast>, | ||
| ) { | ||
| if !ctx.is_lint_enabled(INTERFACE_FILE_NAMING.id()) { | ||
| return; | ||
| } | ||
|
|
||
| // Get first item in file and exit if the unit contains no items | ||
| let Some(first_item) = unit.items.first() else { return }; | ||
|
|
||
| // Get file from first item | ||
| let file = ctx.session().source_map().lookup_source_file(first_item.span.lo()); | ||
|
|
||
| // Get file name from file | ||
| let Some(file_name) = file.name.as_real().and_then(|path| path.file_name()?.to_str()) | ||
| else { | ||
| return; | ||
| }; | ||
|
|
||
| // If file name starts with 'I', skip lint | ||
| if file_name.starts_with('I') { | ||
| return; | ||
| } | ||
|
|
||
| let mut first_interface_span = None; | ||
| for item in unit.items.iter() { | ||
| if let ast::ItemKind::Contract(c) = &item.kind { | ||
| match c.kind { | ||
| ast::ContractKind::Interface => { | ||
| first_interface_span.get_or_insert(c.name.span); | ||
| } | ||
| _ => return, // Mixed file, skip lint | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Emit if file contains ONLY interfaces. Emit only on the first interface. | ||
| if let Some(span) = first_interface_span { | ||
| ctx.emit(&INTERFACE_FILE_NAMING, span); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
nit, we can also simplify as a single let chain here? we should be able to do that if we add a helper to compute the file name with something like:
if let Some(file_name) = file_name(ctx, unit)
&& !file_name.starts_with('I')
&& unit.items.iter().all(|item| {
matches!(item.kind, ast::ItemKind::Contract(c) if c.kind == ast::ContractKind::Interface)
})
&& let Some(ast::ItemKind::Contract(c)) = unit.items.first().map(|item| &item.kind)
{
ctx.emit(&INTERFACE_FILE_NAMING, c.name.span);
}fn file_name<'ast>(ctx: &LintContext<'ast, '_>, unit: &'ast ast::SourceUnit<'ast>) -> Option<&str> {
let first_item_span = unit.items.first()?.span;
let file = ctx.session().source_map().lookup_source_file(first_item_span.lo());
file.name.as_real()?.file_name()?.to_str()
}
Comment on lines
26
to
49
| // Check which types are exempted | ||
| let exceptions = &ctx.config.lint_specific.multi_contract_file_exceptions; | ||
| let should_lint_interfaces = !exceptions.contains(&ContractException::Interface); | ||
| let should_lint_libraries = !exceptions.contains(&ContractException::Library); | ||
| let should_lint_abstract = !exceptions.contains(&ContractException::AbstractContract); | ||
|
|
||
| // Collect spans of all contract-like items, skipping those that are exempted | ||
| let relevant_spans: Vec<_> = unit | ||
| .items | ||
| .iter() | ||
| .filter_map(|item| match &item.kind { | ||
| ast::ItemKind::Contract(c) => { | ||
| let should_lint = match c.kind { | ||
| ast::ContractKind::Interface => should_lint_interfaces, | ||
|
|
||
| ast::ContractKind::Library => should_lint_libraries, | ||
| ast::ContractKind::AbstractContract => should_lint_abstract, | ||
| ast::ContractKind::Contract => true, // Regular contracts are always linted | ||
| }; | ||
| should_lint.then_some(c.name.span) | ||
| } | ||
| _ => None, | ||
| }) | ||
| .collect(); |
Contributor
There was a problem hiding this comment.
can we move the exception logic to an impl block on LintSpecificConfig? this way the code will be much cleaner.
maybe something like:
impl LintSpecificConfig {
/// Checks if a given contract kind is included in the list of exceptions
fn is_exempted(&self, contract_kind: &ast::ContractKind) -> bool {
let exception_to_check = match contract_kind {
ast::ContractKind::Interface => ContractException::Interface,
ast::ContractKind::Library => ContractException::Library,
ast::ContractKind::AbstractContract => ContractException::AbstractContract,
// Regular contracts are always linted
ast::ContractKind::Contract => return false,
};
self.multi_contract_file_exceptions.contains(&exception_to_check)
}
}
Contributor
|
@milosdjurica btw, i haven't tested the my code compiles, those are just suggestions, so you may need to tweak them a bit, but u get the gist of the requested simplifications |
Contributor
Author
|
Hi @0xrusowsky, thanks for suggestions - it's much cleaner now. I have applied them with some small changes, lmk if this is okay now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
.solfile .LinterConfigMotivation
Enforce best practice.
Solution
I have some doubts regarding this, would like to hear your opinion.
My solution count interfaces and libraries too, and it shows linting note only once per file. Let me know if you want something to be changed :)
PR Checklist
Keep up the good work guys, cheers!