Add type_method_receiver_ref_now_mutable lint #1243
Conversation
|
I'm not sure if these may be more suitable for reference_link: |
Frank-III
left a comment
Frank-III
left a comment
There was a problem hiding this comment.
The core logic is correct, and I know you probably use LLM for the lint, but could you take a detailed look at other lints before submitting it?
Also, you should notice that when testing ImplOwner, we usually test struct/enum/union, could you add tests for all of them?
| receiver { | ||
| by_reference @filter(op: "=", value: ["$true"]) | ||
| method_receiver_kind: kind @tag @output | ||
| } |
There was a problem hiding this comment.
should avoid extra space
| } | |
| } |
| path @filter(op: "=", value: ["%path"]) | ||
| public_api @filter(op: "=", value: ["$true"]) | ||
| } | ||
|
|
There was a problem hiding this comment.
remove extra new-line
| receiver { | ||
| by_mut_reference @filter(op: "=", value: ["$true"]) | ||
| kind @filter(op: "=", value: ["%method_receiver_kind"]) | ||
| } |
There was a problem hiding this comment.
could you clean all of these extra space?
| } | |
| } |
There was a problem hiding this comment.
Sure! Thank you for the reviews!
|
|
||
| method { | ||
| method_name: name @output @tag | ||
| public_api_eligible @filter(op: "=", value: ["$true"]) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
But the external files/modules can't call the private methods so won't be effected, right?
| error_message: "A type method's receiver now requires a mutable reference instead of immutable one. Uses of this method that supplied the previous mutability state will be broken.", | ||
| per_result_error_template: Some("method {{join \"::\" path}}::{{method_name}} in {{span_filename}}:{{span_begin_line}}"), | ||
| witness: ( | ||
| hint_template: r#"make the reference mutable for method '{{ join "::" path }}::{{ method_name }}'"# |
There was a problem hiding this comment.
See docs for adding a witness here. https://github.com/obi1kenobi/cargo-semver-checks/blob/main/CONTRIBUTING.md#hint-templates
Does this help clarify what this field should be? I'd love to improve the docs if they're unclear.
There was a problem hiding this comment.
is this good?
witness: (
hint_template: r#"let x: {{join "::" path}} = ...; x.{{ method_name }}(...);"#
)
I noticed it won't be accepted for kinds other than 'Self', can I add custom helper 'replace' to use it as {{replace method_receiver_kind "Self" name}}? or remove the witness
There was a problem hiding this comment.
I'm not familiar enough with the receiver schema that Frank added to answer your second question, but in my opinion, something that only has an &T (so cant call methods on &mut T) may be clearer as a witness:
fn downstream(x: &path::to::Struct) {
x.method(...);
}because it shows greater breakage than "you just have to add mut to the let"
KhaledSayed04
force-pushed
the
add_new_lint
branch
from
f3134c0 to
1be3d0d
Compare
For the written code no. I may chat with it to brainstorm. |
KhaledSayed04
force-pushed
the
add_new_lint
branch
from
1be3d0d to
e1bafa8
Compare
4 participants
Add type_method_receiver_ref_now_mutable lint to solve #1223 issue.