Skip to content

Add Lint/DuplicateMethodSignature rule#697

Merged
Sija merged 3 commits intomasterfrom
add-lint-duplicate-method-definition-rule
Dec 15, 2025
Merged

Add Lint/DuplicateMethodSignature rule#697
Sija merged 3 commits intomasterfrom
add-lint-duplicate-method-definition-rule

Conversation

@Sija
Copy link
Member

@Sija Sija commented Nov 15, 2025

Resolves #658

@Sija Sija added this to the 1.7.0 milestone Nov 15, 2025
@Sija Sija requested a review from nobodywasishere November 15, 2025 04:06
@Sija Sija self-assigned this Nov 15, 2025
@Sija Sija added the rule label Nov 15, 2025
@Sija Sija force-pushed the add-lint-duplicate-method-definition-rule branch 3 times, most recently from 20ccf36 to 93a6039 Compare November 15, 2025 04:35
@straight-shoota
Copy link
Contributor

straight-shoota commented Nov 15, 2025

Matching on mere syntactical likeness seems very fragile. I doubt whether this can be implemented satisfactorily without at least a little bit of semantic analysis (top level visitor).

The detection rate of this rule implementation is very low. Signatures with little alterations such as different names, type restrictions, default values, etc. are very likely in real code bases, but the rule doesn't detect them.

I'm concerned that the rule is misleading because I expect it has an utterly poor detection rate.
I would expect this rule to ensure the codebase is practically free of unwanted duplicate method definitions. But it's unlikely to achieve anything near that in real world scenarios.

@Sija
Copy link
Member Author

Sija commented Nov 15, 2025

Matching on mere syntactical likeness seems very fragile. I doubt whether this can be implemented satisfactorily without at least a little bit of semantic analysis (top level visitor).

That would at least help with issues like the one mentioned by the OP or crystal-lang/crystal#16366.

The detection rate of this rule implementation is very low. Signatures with little alterations such as different names, type restrictions, default values, etc. are very likely in real code bases, but the rule doesn't detect them.

That's intentional, since I was thinking more of cases like the ones mentioned, for which this implementation should be sufficient - like copy-paste errors and simple mistakes, whereas a more complicated implementation would need to leverage semantic analysis to be fully useful.

I'm concerned that the rule is misleading because I expect it has an utterly poor detection rate.
I would expect this rule to ensure the codebase is practically free of unwanted duplicate method definitions. But it's unlikely to achieve anything near that in real world scenarios.

Yeah, that might be a wrong expectation given by the rule name.

@nobodywasishere
Copy link
Contributor

Lint/DuplicateMethodSignature?

@Sija Sija force-pushed the add-lint-duplicate-method-definition-rule branch from 93a6039 to 51adf9c Compare December 14, 2025 16:51
@Sija Sija changed the title Add Lint/DuplicateMethodDefinition rule Add Lint/DuplicateMethodSignature rule Dec 14, 2025
@Sija
Copy link
Member Author

Sija commented Dec 14, 2025

@nobodywasishere Thanks! I've used your naming suggestion 🙏 I think this PR is ready for CR now.

@Sija Sija requested a review from nobodywasishere December 15, 2025 22:01
@Sija Sija merged commit f12176c into master Dec 15, 2025
9 checks passed
@Sija Sija deleted the add-lint-duplicate-method-definition-rule branch December 15, 2025 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warn about two methods with the same definition in the same scope

3 participants