feat: Add lint for missing trait methods #427

zurawiki · 2023-04-12T02:51:18Z

chore: Bump trustfall_rustdoc version to 0.10.0
test: Add trait_method_removed test crate
chore: cargo update
feat: Add lint for missing trait methods

Closes #368

obi1kenobi

This is an excellent start!

The query is almost there, and as I think I mentioned, most of the work is going to be in coming up with good wording for the user-facing error messages and in making good test cases :)

obi1kenobi · 2023-04-12T03:10:46Z

src/lints/trait_method_missing.ron

+                        }
+
+                        method {
+                            method_visibility: visibility_limit @filter(op: "=", value: ["$public"]) @output


This line is not necessary, since trait methods are always public and cannot have restricted visibility of any form.

Suggested change

method_visibility: visibility_limit @filter(op: "=", value: ["$public"]) @output

obi1kenobi · 2023-04-12T03:13:50Z

src/lints/trait_method_missing.ron

+                    }
+                }
+            }
+            current @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) {


Recall that we're aiming for only a single lint to be triggered for any given semver violation.

With this @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) here, I believe it's possible to trigger this lint in a case that doesn't involve removing or renaming a trait method.

Can you think of any such example situations? They'd be great to include in the test suite even after updating the query here.

src/lints/trait_method_missing.ron

obi1kenobi · 2023-04-12T03:17:03Z

src/lints/trait_method_missing.ron

+        "public": "public",
+        "zero": 0,
+    },
+    error_message: "A publicly-visible trait method is no longer visible, or cannot be called by its prior path. A member, method, or item of the trait may have been renamed or removed entirely.",


Let's aim to make this message shorter and easier to read. Right now it's a bit confusing since it references ideas that are not obviously related to this lint.

For example, this lint only looks for methods that can't be found, so we don't need to mention other kinds of items that the trait might contain. Also, since visibility restrictions cannot be applied to trait methods (they are all always pub), "no longer visible" is confusing.

ack, still working on it

obi1kenobi · 2023-04-12T03:18:03Z

src/lints/trait_method_missing.ron

+        "zero": 0,
+    },
+    error_message: "A publicly-visible trait method is no longer visible, or cannot be called by its prior path. A member, method, or item of the trait may have been renamed or removed entirely.",
+    per_result_error_template: Some("trait {{join \"::\" path}}, previously in file {{span_filename}}:{{span_begin_line}}"),


Let's name the problematic method here as well as its trait. Right now we only name the trait, so if more than one method gets removed from the same trait, the user would see a duplicate error message and be quite confused.

zurawiki · 2023-04-12T21:55:30Z

I'm still trying to setup the crate so it can build but it seems that there are conflicting types coming from the fact that multiple trustfall_core crate versions are imported. This seems to be the source of the failure with the linter. I can reproduce this locally, and this issue prevents me from building cargo-semver-checks.

error[E0308]: mismatched types
   --> src/query.rs:299:13
    |
299 | /             results_iter
300 | |                 .map(|res| res.into_iter().map(|(k, v)| (k.to_string(), v)).collect())
301 | |                 .collect::<Vec<BTreeMap<_, _>>>(),
    | |_________________________________________________^ expected enum `trustfall::FieldValue`, found enum `trustfall_core::ir::value::FieldValue`
    |
    = note: enum `trustfall_core::ir::value::FieldValue` and enum `trustfall::FieldValue` have similar names, but are actually distinct types
note: enum `trustfall_core::ir::value::FieldValue` is defined in crate `trustfall_core`
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/trustfall_core-0.4.0-beta.1/src/ir/value.rs:12:1
    |
12  | pub enum FieldValue {
    | ^^^^^^^^^^^^^^^^^^^
note: enum `trustfall::FieldValue` is defined in crate `trustfall_core`
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba1[500](https://github.com/obi1kenobi/cargo-semver-checks/actions/runs/4674097773/jobs/8277971388?pr=427#step:6:501)1f/trustfall_core-0.3.3/src/ir/value.rs:10:1
    |
10  | pub enum FieldValue {
    | ^^^^^^^^^^^^^^^^^^^
    = note: perhaps two different versions of crate `trustfall_core` are being used?

What's the best way to update all dependencies on trustfall code?

obi1kenobi · 2023-04-13T12:01:26Z

I'm still trying to setup the crate so it can build but it seems that there are conflicting types coming from the fact that multiple trustfall_core crate versions are imported. This seems to be the source of the failure with the linter. I can reproduce this locally, and this issue prevents me from building cargo-semver-checks.

It just needed a matching version update for the trustfall dependency together with trustfall_rustdoc. I added it in #428 which should merge shortly, and you'll be able to just merge main to pick it up in a bit.

obi1kenobi · 2023-05-04T18:44:13Z

Just checking, are you still interested in pushing this PR forward? No worries if not.

zurawiki · 2023-05-05T20:29:48Z

Thanks for following up. I'd like to see this through but I unfortunately won't have bandwidth this coming week. I'm happy to let someone else takeover the task if there is interest.

obi1kenobi · 2023-05-05T20:32:16Z

There's no rush, so if you think you'll be able to come back to it in the coming weeks, you're welcome to do that. But also no pressure, it's not a big deal to close this and let someone else take a crack at it.

obi1kenobi · 2023-05-15T22:40:15Z

src/lints/trait_method_missing.ron

+                                filename @output
+                                begin_line @output
+                            }


Suggested change

filename @output

begin_line @output

}

filename @output

begin_line @output

}

Just wanted to help with a quick fix. You're headed in the right direction in general.

zurawiki · 2023-05-25T19:52:42Z

Updated the PR with the feedback. It is now ready for review. Thanks @obi1kenobi for all your guidance here.

edit: I am seeing some CI failures that I'm not sure how to fix given that I have updated the test outputs.

obi1kenobi · 2023-05-25T20:28:34Z

As soon as #455 merges, this should start passing tests. Thanks for putting it together!

- Added a new SemverQuery lint for missing trait methods in `src/lints/trait_method_missing.ron`

- Update and simplify the error message template for the SemverQuery - Add a new trait and remove an old trait in the trait_method_missing test crates

src/lints/trait_method_missing.ron

obi1kenobi · 2023-05-26T01:46:23Z

Thanks for putting this together! The nightly failure is on me, we just need to pull in the adapter for the new format and I'll take care of it a bit later.

obi1kenobi reviewed Apr 12, 2023

View reviewed changes

zurawiki closed this Apr 12, 2023

zurawiki reopened this Apr 12, 2023

obi1kenobi reviewed May 15, 2023

View reviewed changes

zurawiki marked this pull request as ready for review May 25, 2023 19:52

zurawiki added 4 commits May 25, 2023 21:18

test: Add trait_method_removed test crate

ba6fbc5

feat: Add lint for missing trait methods

e8b73e1

- Added a new SemverQuery lint for missing trait methods in `src/lints/trait_method_missing.ron`

fix: Fix trait method visibility filter bug and add span metadata.

1b7a728

fix: Update lint and ensure test cases pass

0d144dd

- Update and simplify the error message template for the SemverQuery - Add a new trait and remove an old trait in the trait_method_missing test crates

obi1kenobi approved these changes May 26, 2023

View reviewed changes

src/lints/trait_method_missing.ron Outdated Show resolved Hide resolved

Update src/lints/trait_method_missing.ron

b442de4

obi1kenobi enabled auto-merge (squash) May 26, 2023 01:45

obi1kenobi merged commit 0c44a60 into obi1kenobi:main May 26, 2023

zurawiki deleted the add-check-1 branch May 26, 2023 03:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add lint for missing trait methods #427

feat: Add lint for missing trait methods #427

zurawiki commented Apr 12, 2023 •

edited

Loading

obi1kenobi left a comment

obi1kenobi Apr 12, 2023

obi1kenobi Apr 12, 2023

zurawiki Apr 12, 2023

obi1kenobi Apr 12, 2023

zurawiki Apr 12, 2023

obi1kenobi Apr 12, 2023

zurawiki Apr 12, 2023

zurawiki commented Apr 12, 2023

obi1kenobi commented Apr 13, 2023

obi1kenobi commented May 4, 2023

zurawiki commented May 5, 2023

obi1kenobi commented May 5, 2023

obi1kenobi May 15, 2023

obi1kenobi May 15, 2023

zurawiki commented May 25, 2023 •

edited

Loading

obi1kenobi commented May 25, 2023

obi1kenobi commented May 26, 2023

feat: Add lint for missing trait methods #427

feat: Add lint for missing trait methods #427

Conversation

zurawiki commented Apr 12, 2023 • edited Loading

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zurawiki commented Apr 12, 2023

obi1kenobi commented Apr 13, 2023

obi1kenobi commented May 4, 2023

zurawiki commented May 5, 2023

obi1kenobi commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zurawiki commented May 25, 2023 • edited Loading

obi1kenobi commented May 25, 2023

obi1kenobi commented May 26, 2023

zurawiki commented Apr 12, 2023 •

edited

Loading

zurawiki commented May 25, 2023 •

edited

Loading