Skip to content

fix(lint): add missing visit methods to LateLintVisitor#14276

Merged
figtracer merged 5 commits intofoundry-rs:masterfrom
solanaXpeter:fix-lint-late-visitor-hooks
Apr 23, 2026
Merged

fix(lint): add missing visit methods to LateLintVisitor#14276
figtracer merged 5 commits intofoundry-rs:masterfrom
solanaXpeter:fix-lint-late-visitor-hooks

Conversation

@solanaXpeter
Copy link
Copy Markdown
Contributor

Motivation

LateLintPass exposes nested item/contract/function/var, modifier, and call-args hooks, but LateLintVisitor only dispatched a subset of them.

This left check_modifier and check_call_args unreachable, and the legacy nested-ID hooks no longer matched the current solar::hir::Visit API.

Solution

Add the missing visitor dispatch for nested item/contract/function/var, modifier, and call args in LateLintVisitor.

Keep the original borrowed check_nested_* hooks as deprecated compatibility shims, and add *_id hooks that receive IDs by value so the late lint API stays compatible while matching the current solar visitor semantics.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Copy Markdown
Collaborator

@mablr mablr left a comment

Choose a reason for hiding this comment

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

sgtm, thanks

I just have potentially a concern about the old deprecated hooks being silently ignored.

A solution may be either:

  • Dispatch both old and new hooks
  • or drop simply the deprecated shims entirely (breaking change, but clearer)

Pending another review

@solanaXpeter
Copy link
Copy Markdown
Contributor Author

@mablr Done, thanks for your review!

Copy link
Copy Markdown
Collaborator

@mablr mablr left a comment

Choose a reason for hiding this comment

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

Hi @solanaXpeter, sorry my prev review was misleading, after looking a bit deeper, I believe the second solution is simpler. I think the hooks can be simply replaced with the corrected by-value signatures. No deprecation needed, smaller diff.

@solanaXpeter
Copy link
Copy Markdown
Contributor Author

@mablr Done. I replaced the nested hooks with the by-value signatures directly and trimmed the extra compatibility layer.

Copy link
Copy Markdown
Collaborator

@figtracer figtracer left a comment

Choose a reason for hiding this comment

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

can we add tests for this ?

@solanaXpeter
Copy link
Copy Markdown
Contributor Author

@figtracer Added test, please re-check, thanks!

Copy link
Copy Markdown
Collaborator

@mablr mablr left a comment

Choose a reason for hiding this comment

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

sgtm

please @figtracer could you also recheck?

Copy link
Copy Markdown
Collaborator

@figtracer figtracer left a comment

Choose a reason for hiding this comment

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

think its good for now this is already an improvement

@figtracer figtracer enabled auto-merge (squash) April 23, 2026 10:43
@figtracer figtracer merged commit a36d95a into foundry-rs:master Apr 23, 2026
16 checks passed
@github-project-automation github-project-automation Bot moved this to Done in Foundry Apr 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants