fix: prevent duplicate references for auto-binding resources #50
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the issue of duplicate references for auto-binding resources by refining both the test cases and the compile-time resource reference logic.
- Updated test cases in document_test.go to inline DocumentLinkParams and added a new test for auto-binding sprites as embedded fields.
- Modified compile.go to include identifier comparisons that prevent duplicate auto-binding references.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/server/document_test.go | Refactored tests to inline DocumentLinkParams and added new test covering auto-binding. |
| internal/server/compile.go | Added identifier comparisons to prevent duplicate auto-binding references. |
Comments suppressed due to low confidence (2)
internal/server/compile.go:1249
- Verify that using pointer equality for defIdent and ident is sufficient for detecting duplicate bindings. If equivalent identifiers might exist as separate instances, consider a more robust equality check.
if defIdent == ident {
internal/server/compile.go:1426
- Ensure that comparing defIdent and ident using '==' accurately prevents duplicate auto-binding references. If identifiers with the same value could be distinct objects, updating the comparison logic may be necessary.
if defIdent == ident {
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
=======================================
Coverage 65.43% 65.43%
=======================================
Files 38 38
Lines 5771 5772 +1
=======================================
+ Hits 3776 3777 +1
Misses 1755 1755
Partials 240 240 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -1418,6 +1422,10 @@ func (s *Server) inspectSpxSoundResourceRefAtExpr(result *compileResult, expr go | |||
| if _, ok := result.spxSoundResourceAutoBindings[obj]; !ok { | |||
| return nil | |||
| } | |||
| defIdent := result.defIdentFor(obj) | |||
| if defIdent == ident { | |||
There was a problem hiding this comment.
It seems a normal business logic? need a test case to cover it.
There was a problem hiding this comment.
Fixes goplus/builder#1471 Signed-off-by: Aofei Sheng <aofei@aofeisheng.com>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes duplicate auto-binding resource references by refactoring how document links are generated in tests and how auto-binding resource references are inspected during compilation. Key changes include:
- Replacing intermediate parameter variables with inline struct literals in document link test cases.
- Adding new tests to verify auto-binding behavior for sprites and sounds.
- Refactoring resource reference inspection in compile.go by using the definition identifier comparison to determine the appropriate reference kind.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/server/document_test.go | Updated test cases for documentLink invocations and auto-binding validation |
| internal/server/diagnostic_test.go | Adjusted asset mappings and diagnostic assertions |
| internal/server/compile.go | Modified auto-binding resource reference logic via defIdent checks |
Comments suppressed due to low confidence (2)
internal/server/compile.go:1240
- The check 'if defIdent == ident' relies on pointer equality, which might be fragile if logically equivalent identifiers are not the same instance. Consider implementing a more robust comparison (e.g., comparing unique identifier properties) to ensure the intended behavior.
defIdent := result.defIdentFor(obj)
internal/server/compile.go:1418
- Similar to the other occurrence, the pointer equality check with 'if defIdent == ident' may not reliably distinguish definition identifiers. A more robust mechanism to compare identifier equality is recommended.
defIdent := result.defIdentFor(obj)
Fixes goplus/builder#1471