Fix configurable default field errors for configurable variables#44403
Fix configurable default field errors for configurable variables#44403iamvirul wants to merge 3 commits intoballerina-platform:masterfrom
Conversation
…n record and object field default values. Add unit test for configurable variable usage in record field default values.
iamvirul
requested review from
MaryamZi,
gimantha,
hasithaa and
sameerajayasoma
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44403 +/- ##
============================================
- Coverage 75.03% 75.03% -0.01%
Complexity 58700 58700
============================================
Files 3601 3601
Lines 227095 227098 +3
Branches 29572 29574 +2
============================================
+ Hits 170392 170393 +1
- Misses 47203 47204 +1
- Partials 9500 9501 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if ((recordFieldDefaultValue || objectFieldDefaultValueRequiringIsolation) && | ||
| Symbols.isFlagOn(flags, Flags.CONFIGURABLE)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is the correct fix.
A configurable variable is implicitly final and immutable, so the check at L1315 should be sufficient. The issue here seems to be that the relevant flags aren't set when this check happens.
For example, the following works
configurable string|string[]? fooConfig = ();
isolated int[] x = [1, 2, 3];
function fn() returns string|string[]? {
lock {
foreach var i in x {
}
// This is allowed because the flags are set by the time this check happens
return fooConfig;
}
}There was a problem hiding this comment.
Hi @MaryamZi,
You're completely right - configurable variables are implicitly readonly, so the check at L1315 should naturally succeed. Because fooConfig is perfectly readonly, the issue in #44284 should have passed safely through that initial finality condition instead of failing as mutable storage.
The root cause of this failure is that SemanticAnalyzer processes type definitions (and their record field default values) before global variables. When the varRefExpr for the configurable variable is evaluated inside a record field's default value, the configurable variable's symbol type hasn't been mutated to its readonly intersection type yet.
Consequently, varRefExpr.getBType() (accessType) in IsolationAnalyzer holds onto the initial, non-readonly type (e.g., string|string[]?). While varRefExpr keeps the stale definition, the variable's underlying symbol symbol.type correctly holds the freshly updated readonly intersection type later on.
I've pushed a cleaner fix to remove the specific bypass for Flags.CONFIGURABLE. Instead, I updated the check in IsolationAnalyzer to honor symbol.type alongside accessType.
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughRefines IsolationAnalyzer early-exit in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
2 participants
Purpose
invalid access of mutable storage in the default value of a record field.readonly.Approach
IsolationAnalyzerto skip mutable-storage diagnostics when a record/object field default references a symbol flagged withFlags.CONFIGURABLE.test-src/statements/variabledef/configurable_record_field_default.baland a newGlobalVarTest.testConfigurableVarInRecordFieldDefaultValue, guarding the reported scenario.Samples
tests/jballerina-unit-test/src/test/resources/test-src/statements/variabledef/configurable_record_field_default.baldemonstrates assigning aconfigurable string|string[]?to a record field default.Remarks
./gradlew :jballerina-unit-test:test --tests org.ballerinalang.test.types.globalvar.GlobalVarTeston JDK 21.Check List
Summary
This pull request fixes a compiler validation issue where configurable module variables were incorrectly treated as mutable storage when used as default values for record or object fields, causing an erroneous diagnostic.
Changes
Outcome
Configurable global variables can now be referenced by record/object field defaults without triggering an incorrect mutable-storage error, removing the need for workarounds like marking such globals readonly.