Fix create_script validator false-positive on constructor invocations#1045
Merged
dsarno merged 2 commits intoCoplayDev:betafrom Apr 7, 2026
Merged
Conversation
The regex-based duplicate method signature check misidentified
`new Type(...)` constructor calls as method declarations, causing
valid C# like `new GameObject("A"); new GameObject("B");` to be
rejected. Capture the return-type token and skip when it is `new`.
Addresses CoplayDev#1044
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Reviewer's GuideAdjusts the ManageScript method-signature regex and handling to ignore constructor invocations misclassified as method declarations, and adds focused unit tests to prevent regressions in the duplicate-method validator. Class diagram for ManageScript duplicate method signature check and related testsclassDiagram
class ManageScript {
+CheckDuplicateMethodSignatures(string contents, System.Collections.Generic.IList~string~ errors)
-IsCSharpKeyword(string identifier) bool
-CountTopLevelParams(string paramList) int
-ExtractParamTypes(string paramList) string
}
class ManageScriptValidationTests {
+DuplicateMethodCheck_ConstructorInvocations_NotFlagged()
+DuplicateMethodCheck_MultipleDistinctConstructors_NotFlagged()
}
ManageScriptValidationTests ..> ManageScript : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdjusted duplicate-method-signature detection to correctly separate captured return type from method name in the regex, skip matches where the captured return type is Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- When checking
returnType == "new", consider usingstring.Equals(returnType, "new", StringComparison.Ordinal)to make the intent explicit and avoid any surprises from culture-sensitive comparisons in the future. - Since the regex now relies on detecting
newas the captured return type to distinguish constructor calls, it may be worth adding one more test that includes a method with thenewmodifier plus nearby constructor invocations to guard against regressions in how modifiers vs. return types are parsed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When checking `returnType == "new"`, consider using `string.Equals(returnType, "new", StringComparison.Ordinal)` to make the intent explicit and avoid any surprises from culture-sensitive comparisons in the future.
- Since the regex now relies on detecting `new` as the captured return type to distinguish constructor calls, it may be worth adding one more test that includes a method with the `new` modifier plus nearby constructor invocations to guard against regressions in how modifiers vs. return types are parsed.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Use string.Equals with StringComparison.Ordinal for the "new" check. Add test combining `public new void Init()` with nearby constructor invocations to guard against modifier vs return-type parsing regressions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ManageScript.csmisidentifiednew Type(...)constructor calls as method declarations —newwas matched as the return type and the type name as the method namenew(which can never be a valid return type in a real method declaration)Test plan
DuplicateMethodCheck_ConstructorInvocations_NotFlagged— verifiesnew GameObject("A"); new GameObject("B");no longer triggers false positiveDuplicateMethodCheck_MultipleDistinctConstructors_NotFlagged— verifies mixednew MaterialPropertyBlock()+new GameObject()calls pass🤖 Generated with Claude Code
Summary by Sourcery
Prevent the script validation duplicate-method check from misidentifying constructor invocations as duplicate method declarations.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
newkeyword appear, so unrelatednewusages no longer trigger duplicate-method errors.Tests
newconstructor calls and derived-classnewmodifiers to validate the fix.