exercism / rust-analyzer

A static analyzer for Rust
GNU Affero General Public License v3.0
10 stars 9 forks source link

suggest: refactor analyzer for greater flexibility #11

Closed coriolinus closed 5 years ago

coriolinus commented 5 years ago

The analyzer currently matches full ASTs:

https://github.com/exercism/rust-analyzer/blob/004154cad987142a227a54fb9e3f86bc0de4ec1d/src/analyzers/reverse_string/mod.rs#L53-L64

This works, but is inflexible: it is currently impossible to suggest both implementing the bonus test and also removing use statements. We could refactor for greater flexibility by writing this more like a linter.

Detailed proposal:

  1. Add a static LINTS: Vec<dyn Fn(&File, &mut AnalysisOutput) -> Result<()>>
  2. Populate this list with functions which look for particular features of the input file. For example:

    • one might look for the presence of use statements, and add a suggestion that those aren't necessary in Rust 2018
    • one might look for the absence of use of the unicode_segmentation crate, and suggest that this would be an easy way to implement the bonus feature
    • one might compare the body of the reverse function (ignoring the variable name) with the ideal output, and approve if it is optimal

    These functions have the option to return an error, aborting the parse, but generally they should be OK.

  3. Refactor the main Analyze implementation such that by default, it has a basic "refer to mentor" output. Every lint in the LINTS list is run, possibly modifying the output. If any lint returns an Err, the break out of the lints iteration and return an appropriate error.
  4. Once every lint has run, simply return the fully-mutated AnalysisOutput.
  5. Remove most of the existing PreparedOutput stuff, which should no longer be needed or used.

I believe that this architecture would be sufficient to close #5.

ZapAnton commented 5 years ago

Nice suggestion! This is more or less how I planned to expand the reverse-string analyzer, just need to learn how to effectively traverse syn AST. Also I would try to avoid mutable AnalysisOutput and use builder pattern with every build function as a separate lint, but these are details.

coriolinus commented 5 years ago

just need to learn how to effectively traverse syn AST.

Looks pretty straightforward: just iterate through the File.items. Detecting whether there are use statements could be done like

let has_use = file.items.iter().any(|item| if let Item::Use(_) = item) { true } else { false});

I would try to avoid mutable AnalysisOutput and use builder pattern

Up to you. I think that in this case, using a mutable AnalysisOutput will result in less code, which is therefore easier to read and write, but I agree that it's a detail up to the implementer.

ZapAnton commented 5 years ago

Thanks, will look into it later tonight