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

Consider ESLint integration? #193

Open
replete opened this issue Dec 18, 2024 · 7 comments
Open

Consider ESLint integration? #193

replete opened this issue Dec 18, 2024 · 7 comments

Comments

@replete
Copy link

replete commented Dec 18, 2024

I wanted to express how I'd like to integrate this tool with ESLint, to prompt discussion and maybe a future feature. I couldn't find any search results relating to eslint.

For example on a webapp I'm using airbnb rules with ESLint, and fta looks like an ideal to plug in as an ESLint rule to provide additional rules relating to code complexity. e.g. configurable warnings/errors for files above difficulty/complexity/fta_score.

@sgb-io
Copy link
Owner

sgb-io commented Dec 19, 2024

Hi @replete yes this is something that has occurred to me previously. I think in order to become a useful eslint plugin, we would need to solve #24 first.

Since the halsted output is a linear complexity measure, it doesn't really make sense to say "there is an error here" at a specific piece of code. Instead, it could potentially make sense to draw attention to clusters of complexity within a file, which in theory could be captured, but again would not be a binary good-or-bad outcome.

Another school of thought is that an FTA ESLint plugin simply wraps the cli in order to trip the score_cap (see: https://ftaproject.dev/docs/configuration#score_cap) when a particular file breaches it (and therefore the plugin acts as an eslint-friendly good-or-bad outcome, on a whole-file basis).

Let me know if you have other ideas!

@replete
Copy link
Author

replete commented Dec 26, 2024

I did consider that it would be relatively straightforward to pipe in a per-file score to my ESLint configuration, but then wondered how useful it would be in the IDE to provide a complexity warning for the entire file - at least in the context of a large in-transition codebase. On a new codebase this would be a great starting point.

I believe you are right, it would be far more useful and practical to cluster complexity warnings to code blocks. This ability would make for an excellent addition to an ESLint configuration for an aligned codebase, with a bit of contextual tuning.

Covering static analysis of JS is a bit of a minefield, many projects have been abandoned, for instance. I used Plato many years ago. SonarJS has an ESLint plugin and seems prominent in the space, but the dependencies are huge to a degree I'm that uncomfortable using it on a large project that needs it the most.

I'm not sure I can help much, but I'll be keeping an eye on this project and will try to implement it in my next side project and contribute where I can. I have no doubt that an ESLint plugin implementing this tool would become very popular.

Thanks for developing this great project. 👍

@sgb-io
Copy link
Owner

sgb-io commented Dec 28, 2024

The answer to this could be to extend crates/fta/src/halstead/mod.rs to capture complexity-per-line number. Something like this:

struct AstAnalyzer {
    unique_operators: HashSet<String>,
    unique_operands: HashSet<String>,
    total_operators: usize,
    total_operands: usize,

    // New field: track line-based data
    // line -> (operators_count, operands_count, distinct_operators, distinct_operands)
    per_line_data: HashMap<usize, LineData>,
}

#[derive(Debug, Default)]
struct LineData {
    operators: HashSet<String>,
    operands: HashSet<String>,
    total_operators: usize,
    total_operands: usize,
}

Then, when walking through the AST:

        match expr {
            Expr::Bin(binary_expr) => {
                // Existing logic
                self.unique_operators.insert(binary_expr.op.to_string());
                self.total_operators += 1;

                // track locally
                let mut line_stats = self.per_line_data
                    .entry(line_number)
                    .or_insert_with(LineData::default);
                line_stats.operators.insert(binary_expr.op.to_string());
                line_stats.total_operators += 1;

                binary_expr.left.visit_with(self);
                binary_expr.right.visit_with(self);
            }
            // ... etc.
        }

Then, in theory, exposing per_line_data provides something useful that could be wired in to ESLint

@AnthonyAstige
Copy link

I threw together eslint-plugin-fta for per-file analysis 😄

I was hoping it'd be faster (relative to other ESLint plugins) but it's not bad perhaps considering it's doing full file analysis.

Relevant Code / Analysis

This looks like the slow line. I could likely implement shared state to speed up full repository analysis. However I think this wouldn't help the in editor use case.

Speed noticed testing my repo

Running eslint-plugin-fta on my, 600+ file, 1000+ rule (from eslint-plugin-canonical), repository it appears to be 5-10x slower than any other ESLint rule.

Image

Simple test

From this it seems there may be some warmup or caching going on because it seems to only be "slow" on the first rule in the first file hit.

Image Image

@AnthonyAstige
Copy link

AnthonyAstige commented Mar 15, 2025

Okay I implemented pre-loading / caching. It is a bit of a hack but I think the best that can be done right now for a regular ESLint plugin given my understanding:

  • ESLint architecture is meant to scan one file (or pieces thereof) at a time
  • FTA API is meant to scan a projectPath with many files

Benchmarking results

From my unscientific benchmarking, it's way faster for full repository analysis
Image

At the cost of a slower for single file analysis (which I expect to get worse as a repository grows)
Image

Since it can't seem to go below 80ms for a single file, and the full repo change is so drastic, I'll leave the bulk method for now. It might just be the startup time of the FTA CLI, if that could be improved it may make single file analysis viable.

@sgb-io
Copy link
Owner

sgb-io commented Mar 15, 2025

Nice work @AnthonyAstige! I think we could speed things up by exposing a package designed to parse 1 file at a time. You're right that the FTA CLI is designed to process a whole project directory.

The fta-wasm package does this (1 file at a time) - fta-wasm npm package / rust crate entrypoint

Potential solutions could be:

  1. Publish an additional binary package designed to be used ad-hoc against 1 file (or arbitrary text input) e.g. fta-analyze. This should be fairly simple as we already have the API from the fta-wasm crate -- just need to expose it as another binary and npm package
  2. Make the eslint plugin call the wasm plugin (but maybe there could be overhead of calling a wasm binary from Node, I am not experienced with that). We use this package in the FTA website playground - see example.

I think option (1) is likely the best approach

@AnthonyAstige
Copy link

AnthonyAstige commented Mar 15, 2025

@sgb-io Thanks :)

(1) sounds good to me and I can try it if you can expose the endpoint~

Perf concerns of single file parsing

I am slightly concerned how a single file parser will perform on large codebases with lots of files when doing a full ESLint run. It'll still be kicking off the process even run, which I suspect 80 [ms] of the 130 [ms] I see comes from, which would somewhat get multiplied over the number of files. This could be side-stepped by adding rule options and/or multiple rules with difference performance characteristics. Adds UX complexity though. I can do sanity check "benchmarks" again though in any case to see if that's necessary first once I have a single file endpoint to call.

Current Mitigations

I mitigate my use case of the perf issue by disabling the rule on file save (I have a pre-save hook in my editor that blocks saving, so even 10ms would have a cost that could add up). That said I don't need this rule when saving; I only need auto-fixing. I'm not sure on the perf with my LSP in my editor, but that's async and non-blocking at least. It probably is adding a delay but I don't know offhand how to test the LSP directly only see the ESLint rule timing running from the shell. That said from our understanding here a single file mode would likely improve this use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants