fix(config): prioritize nested remappings#15800
Conversation
|
Potential regression to cover: this change can narrow a package-root remapping globally and break imports outside the configured source directory. Example fixture: Before this change, auto-detection yields Could we add coverage for mixed package layouts where Solidity files are imported from the package root, |
grandizzy
left a comment
There was a problem hiding this comment.
pls consider #15800 (comment) Projects are using remappings in many ways and our unit tests doesn't cover them so likely a fix introduce regressions. Some other scenarios to take into account for this fix:
- Cross-dependency leakage: two parents defining the same alias differently remain uncontextualized; the globally “closest” path wins, potentially compiling one parent against the other’s dependency.
- Dependency-controlled hijack: a transitive dependency can narrow an auto-detected alias to a deeper, unexpected directory—similar to the security concern in #9146.
- Scoped/versioned packages: filtering self-remappings by directory basename fails when folder and import names differ, e.g. foo-1.2.0 declaring
foo/or@scope/foo/. - Symlinks/nonexistent targets:
Path::starts_with()is lexical when canonicalization fails, so..paths may incorrectly qualify as descendants. - Performance: every nested
foundry.tomlis loaded twice, adding configuration and filesystem work across large dependency trees.
|
Remaining contextual-remapping regressions to cover: 1. Root file-level context [profile.default]
remappings = ["src/A.sol:@x/=lib/x/"]The context becomes 2. Dependency file-level context The context becomes 3. Source-prefix context without a directory [profile.default]
remappings = ["src/generated:@x/=lib/x/"]Contexts use source-unit prefix matching, so this currently applies to These cases come from unconditionally treating contexts as directories. Could we preserve exact user-supplied contexts and only add directory boundaries to synthesized dependency contexts? |
There was a problem hiding this comment.
ty! Please check variations in #15800 (comment) 🙏
…ping-precedence Amp-Thread-ID: https://ampcode.com/threads/T-019f8aa2-43b7-7606-895f-642a797b946d Co-authored-by: Amp <amp@ampcode.com> # Conflicts: # crates/config/src/lib.rs
| for remapping in remappings { | ||
| if !authoritative.iter().any(|existing| { | ||
| remapping_context_covers(root, existing, &remapping) | ||
| && remapping_name_covers(&existing.name, &remapping.name) |
There was a problem hiding this comment.
Blocker: This directional check keeps the inverse overlap (pkg/sub/ global + lib/dep/:pkg/ scoped). foundry-compilers then resolves a dependency import pkg/sub/X.sol with the first/global mapping, while solc/Solar choose the longer dependency context. Extending narrow_root_remapping_preserves_broad_dependency_fallback with that import makes forge build fail because the graph and compiler load different files. Please cover this overlap and make both resolver paths select the same mapping.
| groups.insert(remapping.context.clone()); | ||
| } | ||
| for (group, remappings) in groups { | ||
| for group in groups { |
There was a problem hiding this comment.
--pretty no longer groups the output: the first loop prints every mapping, then this loop emits bare context headers after them. This contradicts the flag help. Could we emit each header while iterating remappings in original order so the mappings remain attached to their group?
Nested dependency remappings can point to a source directory such as
contracts/orsrc/, but filesystem detection may incorrectly replace them with the package root.Preserve configured nested remappings when they refine the detected package path, while keeping root and direct dependency precedence unchanged.
Fixes #6431.