Skip to content
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.

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

fix(workspace): remove caching of file features #2939

Merged
merged 2 commits into from
May 21, 2024

Conversation

ematipico
Copy link
Member

Summary

Closes #2781

The issue was the aggressive caching of FileFeatureResult, maybe designed with poor judgement from my side.

Here's the issue explained: when a file is opened, the LSP request diagnostics from a certain file. We pull diagnostics using these feature set:

        let file_features = self.workspace.file_features(SupportsFeatureParams {
            features: FeaturesBuilder::new()
                .with_linter()
                .with_organize_imports()
                .build(),
            path: biome_path.clone(),
        })?;

A formatter doesn't have proper diagnostics, so we exclude it from the features. Since the formatter feature isn't requested, the default value is used, which is Supported because ignore part is checked only on a parameter basis.

However, this was then cached inside our workspace.

When calling the formatting, we pull the formatter feature:

    let file_features = session.workspace.file_features(SupportsFeatureParams {
        path: biome_path.clone(),
        features: FeaturesBuilder::new().with_formatter().build(),
    })?;

Which is flagged as Supported because we retrieve the cached value.

As for a solution, for now, I removed the cache.

I plan to re-introduce the caching again, but I plan to rewrite the logic is a more pure way.

Test Plan

I manually tested it.

@ematipico ematipico requested review from a team May 21, 2024 15:10
@github-actions github-actions bot added A-Project Area: project A-Changelog Area: changelog labels May 21, 2024
@ematipico ematipico merged commit f30f766 into main May 21, 2024
13 checks passed
@ematipico ematipico deleted the fix/formatter-ignore-lsp branch May 21, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Project Area: project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 formatOnSave does not respect formatter.ignore
2 participants