Skip to content

fix(oxlintrc): resolve relative plugin specifiers before merging extended configs #14562

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

Jump to bottom
Merged
merged 4 commits into from
Oct 15, 2025
Merged

fix(oxlintrc): resolve relative plugin specifiers before merging extended configs #14562

merged 4 commits into from
Oct 15, 2025

Conversation

@lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Oct 13, 2025 •
edited
Loading

What's going wrong

  • Relative specifiers need to joined with their config's directory path.
  • When extended configs are merged. we drop the "other" config's path.
  • In this "extends" case, the plugins from the "other" config are resolved relative to the "self" config's path. That shouldn't happen.

Fix

  • All relative specifiers are resolved before the merge.
  • Since full platform-specific (correct) paths to the plugin are logged when loading fails, this won't work well with the snapshot tests.
image

I am vacillating on this solution. Maybe external_plugin should be an enum.

@lilnasy lilnasy marked this pull request as ready for review October 13, 2025 17:06
@lilnasy lilnasy requested a review from camc314 as a code owner October 13, 2025 17:06
@github-actions github-actions bot added A-linter Area - Linter A-cli Area - CLI C-bug Category - Bug labels Oct 13, 2025
camc314
camc314 reviewed Oct 13, 2025
View reviewed changes
crates/oxc_linter/src/config/oxlintrc.rs Outdated
Comment on lines 159 to 174
let mut normalized = FxHashSet::default();
normalized.reserve(external_plugins.len());

// we can safely consume external_plugins while iterating over it since its going to be replaced soon
for plugin in std::mem::take(external_plugins) {
let is_relative = plugin.starts_with("./") || plugin.starts_with("../");
if is_relative {
let absolute = config_dir.join(&plugin);
// TODO: non utf8 path compatibility
normalized.insert(absolute.to_string_lossy().into_owned());
} else {
normalized.insert(plugin);
}
}

config.external_plugins = Some(normalized);
Copy link
Contributor

@camc314 camc314 Oct 13, 2025

Choose a reason for hiding this comment

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

i think we can just mutate it in place, rather than allocating a new hash set

Copy link
Contributor Author

@lilnasy lilnasy Oct 14, 2025

Choose a reason for hiding this comment

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

i am new to rust, thanks for the tip!

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 13, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #14562 will not alter performance

Comparing lilnasy:c/10-13-add_repro (f07a3d2) with main (2cd53c5)1

Summary

✅ 4 untouched
⏩ 33 skipped2

Footnotes

  1. No successful run was found on c/10-13-add_repro (32cceaf) during the generation of this report, so main (2cd53c5) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

  2. 33 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

camc314
camc314 approved these changes Oct 15, 2025
View reviewed changes
Copy link
Contributor

@camc314 camc314 left a comment

Choose a reason for hiding this comment

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

thank you!

@camc314 camc314 merged commit 3c97e2d into oxc-project:c/10-13-add_repro Oct 15, 2025
20 of 21 checks passed
camc314 added a commit that referenced this pull request Oct 15, 2025
@lilnasy @camc314
fix(oxlintrc): resolve relative plugin specifiers before merging exte…
444cf06 
…nded configs (#14562)

- Fixes #14478
## What's going wrong
- Relative specifiers need to joined with their config's directory path.
- When extended configs are merged. we drop the "other" config's path.
- In this "extends" case, the plugins from the "other" config are
resolved relative to the "self" config's path. That shouldn't happen.

## Fix
- All relative specifiers are resolved _before_ the merge.
- Since full platform-specific (correct) paths to the plugin are logged
when loading fails, this won't work well with the snapshot tests.

<img width="898" height="249" alt="image"
src="https://github.com/user-attachments/assets/7e4d0fa1-c0c1-4dc5-8135-dbf663900902"
/>

I am vacillating on this solution. Maybe external_plugin should be an
enum.

---------

Co-authored-by: Cameron Clark <cameron.clark@hey.com>
camc314 added a commit that referenced this pull request Oct 15, 2025
@lilnasy @camc314
fix(oxlintrc): resolve relative plugin specifiers before merging exte…
34782ff 
…nded configs (#14562)

- Fixes #14478
## What's going wrong
- Relative specifiers need to joined with their config's directory path.
- When extended configs are merged. we drop the "other" config's path.
- In this "extends" case, the plugins from the "other" config are
resolved relative to the "self" config's path. That shouldn't happen.

## Fix
- All relative specifiers are resolved _before_ the merge.
- Since full platform-specific (correct) paths to the plugin are logged
when loading fails, this won't work well with the snapshot tests.

<img width="898" height="249" alt="image"
src="https://github.com/user-attachments/assets/7e4d0fa1-c0c1-4dc5-8135-dbf663900902"
/>

I am vacillating on this solution. Maybe external_plugin should be an
enum.

---------

Co-authored-by: Cameron Clark <cameron.clark@hey.com>
camc314 added a commit that referenced this pull request Oct 15, 2025
@lilnasy @camc314
fix(oxlintrc): resolve relative plugin specifiers before merging exte…
f0c251e 
…nded configs (#14562)

- Fixes #14478
## What's going wrong
- Relative specifiers need to joined with their config's directory path.
- When extended configs are merged. we drop the "other" config's path.
- In this "extends" case, the plugins from the "other" config are
resolved relative to the "self" config's path. That shouldn't happen.

## Fix
- All relative specifiers are resolved _before_ the merge.
- Since full platform-specific (correct) paths to the plugin are logged
when loading fails, this won't work well with the snapshot tests.

<img width="898" height="249" alt="image"
src="https://github.com/user-attachments/assets/7e4d0fa1-c0c1-4dc5-8135-dbf663900902"
/>

I am vacillating on this solution. Maybe external_plugin should be an
enum.

---------

Co-authored-by: Cameron Clark <cameron.clark@hey.com>
camc314 added a commit that referenced this pull request Oct 15, 2025
@lilnasy @camc314
fix(oxlintrc): resolve relative plugin specifiers before merging exte…
69c8965 
…nded configs (#14562)

- Fixes #14478
## What's going wrong
- Relative specifiers need to joined with their config's directory path.
- When extended configs are merged. we drop the "other" config's path.
- In this "extends" case, the plugins from the "other" config are
resolved relative to the "self" config's path. That shouldn't happen.

## Fix
- All relative specifiers are resolved _before_ the merge.
- Since full platform-specific (correct) paths to the plugin are logged
when loading fails, this won't work well with the snapshot tests.

<img width="898" height="249" alt="image"
src="https://github.com/user-attachments/assets/7e4d0fa1-c0c1-4dc5-8135-dbf663900902"
/>

I am vacillating on this solution. Maybe external_plugin should be an
enum.

---------

Co-authored-by: Cameron Clark <cameron.clark@hey.com>
camc314 added a commit that referenced this pull request Oct 15, 2025
@lilnasy @camc314
fix(oxlintrc): resolve relative plugin specifiers before merging exte…
bc14d64 
…nded configs (#14562)

- Fixes #14478
## What's going wrong
- Relative specifiers need to joined with their config's directory path.
- When extended configs are merged. we drop the "other" config's path.
- In this "extends" case, the plugins from the "other" config are
resolved relative to the "self" config's path. That shouldn't happen.

## Fix
- All relative specifiers are resolved _before_ the merge.
- Since full platform-specific (correct) paths to the plugin are logged
when loading fails, this won't work well with the snapshot tests.

<img width="898" height="249" alt="image"
src="https://github.com/user-attachments/assets/7e4d0fa1-c0c1-4dc5-8135-dbf663900902"
/>

I am vacillating on this solution. Maybe external_plugin should be an
enum.

---------

Co-authored-by: Cameron Clark <cameron.clark@hey.com>
camc314 added a commit that referenced this pull request Oct 15, 2025
@camc314 @lilnasy
fix(oxlintrc): resolve relative plugin specifiers before merging exte…
ed624c5 
…nded configs (#14562)

- Fixes #14478
- Relative specifiers need to joined with their config's directory path.
- When extended configs are merged. we drop the "other" config's path.
- In this "extends" case, the plugins from the "other" config are
resolved relative to the "self" config's path. That shouldn't happen.

- All relative specifiers are resolved _before_ the merge.
- Since full platform-specific (correct) paths to the plugin are logged
when loading fails, this won't work well with the snapshot tests.

<img width="898" height="249" alt="image"
src="https://github.com/user-attachments/assets/7e4d0fa1-c0c1-4dc5-8135-dbf663900902"
/>

I am vacillating on this solution. Maybe external_plugin should be an
enum.

---------

Co-authored-by: Arsh <emmagoldmanvevo+github@pm.me>
camc314 added a commit that referenced this pull request Oct 15, 2025
@camc314 @lilnasy
fix(oxlintrc): resolve relative plugin specifiers before merging exte…
26bac00 
…nded configs (#14562)

- Fixes #14478
- Relative specifiers need to joined with their config's directory path.
- When extended configs are merged. we drop the "other" config's path.
- In this "extends" case, the plugins from the "other" config are
resolved relative to the "self" config's path. That shouldn't happen.

- All relative specifiers are resolved _before_ the merge.
- Since full platform-specific (correct) paths to the plugin are logged
when loading fails, this won't work well with the snapshot tests.

<img width="898" height="249" alt="image"
src="https://github.com/user-attachments/assets/7e4d0fa1-c0c1-4dc5-8135-dbf663900902"
/>

I am vacillating on this solution. Maybe external_plugin should be an
enum.

---------

Co-authored-by: Arsh <emmagoldmanvevo+github@pm.me>
camc314 added a commit that referenced this pull request Oct 15, 2025
@camc314 @lilnasy
fix(oxlintrc): resolve relative plugin specifiers before merging exte…
6c30cc9 
…nded configs (#14562)

- Fixes #14478
- Relative specifiers need to joined with their config's directory path.
- When extended configs are merged. we drop the "other" config's path.
- In this "extends" case, the plugins from the "other" config are
resolved relative to the "self" config's path. That shouldn't happen.

- All relative specifiers are resolved _before_ the merge.
- Since full platform-specific (correct) paths to the plugin are logged
when loading fails, this won't work well with the snapshot tests.

<img width="898" height="249" alt="image"
src="https://github.com/user-attachments/assets/7e4d0fa1-c0c1-4dc5-8135-dbf663900902"
/>

I am vacillating on this solution. Maybe external_plugin should be an
enum.

---------

Co-authored-by: Arsh <emmagoldmanvevo+github@pm.me>
camc314 added a commit that referenced this pull request Oct 15, 2025
@camc314 @lilnasy
fix(oxlintrc): resolve relative plugin specifiers before merging exte…
55becc5 
…nded configs (#14562)

- Fixes #14478
- Relative specifiers need to joined with their config's directory path.
- When extended configs are merged. we drop the "other" config's path.
- In this "extends" case, the plugins from the "other" config are
resolved relative to the "self" config's path. That shouldn't happen.

- All relative specifiers are resolved _before_ the merge.
- Since full platform-specific (correct) paths to the plugin are logged
when loading fails, this won't work well with the snapshot tests.

<img width="898" height="249" alt="image"
src="https://github.com/user-attachments/assets/7e4d0fa1-c0c1-4dc5-8135-dbf663900902"
/>

I am vacillating on this solution. Maybe external_plugin should be an
enum.

---------

Co-authored-by: Arsh <emmagoldmanvevo+github@pm.me>
@lilnasy lilnasy deleted the c/10-13-add_repro branch October 21, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@camc314 camc314 camc314 approved these changes

Assignees

No one assigned

Labels

A-cli Area - CLI A-linter Area - Linter C-bug Category - Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lilnasy @camc314