Repository navigation
Conversation
…essible object initialization
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds an initializer-specific diagnostic for inaccessible object initialization. Shared diagnostic handling now serves ChangesNon-accessible object initializer diagnostic
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/CodeAnalyzer.java (1)
3646-3660: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicate logic with
SymbolResolver.lookupMemberSymbol.The exact same suffix-check/substring logic to detect a non-accessible init method and derive the object name is duplicated here and in
SymbolResolver.lookupMemberSymbol(lines 981-987). Consider extracting a shared helper (e.g., a static method returning the diagnostic code + object name, or placed inNames/a diagnostics utility) to avoid the two implementations drifting apart.♻️ Suggested consolidation
- private void checkAccessSymbol(BSymbol symbol, PackageID pkgID, Location position) { - if (symbol == null) { - return; - } - - if (!pkgID.equals(symbol.pkgID) && !Symbols.isPublic(symbol)) { - if (symbol.name.value.endsWith("." + Names.USER_DEFINED_INIT_SUFFIX.value)) { - String objName = symbol.name.value.substring(0, - symbol.name.value.length() - Names.USER_DEFINED_INIT_SUFFIX.value.length() - 1); - dlog.error(position, DiagnosticErrorCode.ATTEMPT_INITIALIZE_NON_ACCESSIBLE_OBJECT, objName); - } else { - dlog.error(position, DiagnosticErrorCode.ATTEMPT_REFER_NON_ACCESSIBLE_SYMBOL, symbol.name); - } - } - } + private void checkAccessSymbol(BSymbol symbol, PackageID pkgID, Location position) { + if (symbol == null) { + return; + } + + if (!pkgID.equals(symbol.pkgID) && !Symbols.isPublic(symbol)) { + DiagnosticUtils.reportNonAccessibleSymbol(dlog, position, symbol); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/CodeAnalyzer.java` around lines 3646 - 3660, The init-method accessibility check in CodeAnalyzer.checkAccessSymbol duplicates the same suffix parsing logic already used in SymbolResolver.lookupMemberSymbol, so extract that non-accessible-init detection and object-name derivation into a shared helper. Reuse the helper from both checkAccessSymbol and lookupMemberSymbol, keeping the diagnostic selection and extracted object name in one place so the behavior stays consistent and does not drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/CodeAnalyzer.java`:
- Around line 3646-3660: The init-method accessibility check in
CodeAnalyzer.checkAccessSymbol duplicates the same suffix parsing logic already
used in SymbolResolver.lookupMemberSymbol, so extract that non-accessible-init
detection and object-name derivation into a shared helper. Reuse the helper from
both checkAccessSymbol and lookupMemberSymbol, keeping the diagnostic selection
and extracted object name in one place so the behavior stays consistent and does
not drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd433f30-5e1b-4f13-8149-060e46efdb0a
📒 Files selected for processing (5)
compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticErrorCode.javacompiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/CodeAnalyzer.javacompiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolResolver.javacompiler/ballerina-lang/src/main/resources/compiler.propertiestests/jballerina-unit-test/src/test/java/org/ballerinalang/test/object/ObjectInitializerTest.java
|
Hi @MaryamZi @gimantha @hasithaa @sameerajayasoma — this PR has been open for a bit and is ready for review. It's a small compiler fix for #22100 (improving the error message when initializing an object with a non-accessible init). All checks are green. Could someone take a look when you have a chance? Thanks! |
| if (symbol.name.value.endsWith("." + Names.USER_DEFINED_INIT_SUFFIX.value)) { | ||
| String objName = symbol.name.value.substring(0, | ||
| symbol.name.value.length() - Names.USER_DEFINED_INIT_SUFFIX.value.length() - 1); | ||
| dlog.error(position, DiagnosticErrorCode.ATTEMPT_INITIALIZE_NON_ACCESSIBLE_OBJECT, objName); | ||
| } else { | ||
| dlog.error(position, DiagnosticErrorCode.ATTEMPT_REFER_NON_ACCESSIBLE_SYMBOL, symbol.name); | ||
| } |
There was a problem hiding this comment.
You have added the same code in both places. Can you check if you ca reuse the code block?
Also why are we logging the same error from two different phases (CodeAnalyzer and SymbolResolver)?
There was a problem hiding this comment.
You have added the same code in both places. Can you check if you ca reuse the code block? Also why are we logging the same error from two different phases (CodeAnalyzer and SymbolResolver)?
Hi @gimantha,
Thanks for pointing this out! I investigated both call sites to trace how symbol access is checked during compilation:
Why SymbolResolver was redundant
- When compiling an object initialization expression like new pkg:Student(), TypeChecker resolves the public type Student and directly attaches the initializer function symbol (((BObjectTypeSymbol)actualType.tsymbol).initializerFunc.symbol) to initInvocation.symbol.
- It bypasses SymbolResolver.lookupMemberSymbol for the init method lookup.
Later, during the CodeAnalyzer phase, checkAccess() visits the BLangInvocation node for initInvocation and delegates to checkAccessSymbol(). This is where the accessibility of non-public init methods across - package boundaries is actually evaluated and logged.
Resolution
- Since SymbolResolver.lookupMemberSymbol is never reached for init access during new expressions, I have removed the redundant logic from SymbolResolver.java and kept the check exclusively in CodeAnalyzer.java.
Existing unit tests pass cleanly with this change.
There was a problem hiding this comment.
Lets extract out the common logic to a different method and reuse it in both places
There was a problem hiding this comment.
Lets extract out the common logic to a different method and reuse it in both places
@gimantha Thanks! I have extracted the shared diagnostic logic into a helper method DiagnosticUtils.logNonAccessibleSymbolError() within org.wso2.ballerinalang. compiler.semantics.analyzer and updated both CodeAnalyzer.java (checkAccessSymbol) and SymbolResolver.java (lookupMemberSymbol) to call it.
Signed-off-by: kavix <kavix@yahoo.com>
Extracted non-accessible symbol error logging into a new `DiagnosticUtils` helper and replaced duplicated logic in `CodeAnalyzer` and `SymbolResolver`. This centralizes diagnostic behavior and ensures inaccessible object initializer references consistently report `ATTEMPT_INITIALIZE_NON_ACCESSIBLE_OBJECT` instead of the generic symbol access error. Signed-off-by: kavix <kavix@yahoo.com>
…iagnostic Signed-off-by: kavix <kavix@yahoo.com>
2 participants
Purpose
The compiler complains when trying to initialize an object which has a module-level
initmethod, which is accepted behaviour for objects across different packages. However, the error messageattempt to refer to non-accessible symbol 'Obj.init'is not very user-friendly, asinitis an internal compiler detail.This PR provides a more user-friendly compile-time error message:
attempt to initialize object 'Obj' with a non-accessible initialization method.Fixes #22100
Approach
ATTEMPT_INITIALIZE_NON_ACCESSIBLE_OBJECTinDiagnosticErrorCode.java.compiler.properties.checkAccessSymbolinCodeAnalyzer.javaandlookupMemberSymbolinSymbolResolver.javato check if a non-accessible symbol name ends with theNames.USER_DEFINED_INIT_SUFFIX(.init).initmethod, it strips the.initsuffix to get the object name and emits the newATTEMPT_INITIALIZE_NON_ACCESSIBLE_OBJECTerror; otherwise, it falls back to the genericATTEMPT_REFER_NON_ACCESSIBLE_SYMBOLerror.Samples
Remarks
Check List
Improved compiler diagnostics for object initialization with a non-accessible
initmethod. The compiler now reports a clear, user-facing error without exposing internal symbol names. Diagnostic handling was centralized, and related unit tests were updated.