Fix BFields of generated immutable BRecords not being marked as SymbolFlags.READONLY
#42521
Conversation
Thushara-Piyasekara
force-pushed
the
fix-error-code-inconsistency
branch
2 times, most recently
from
98020f7
to
a96071c
Compare
Thushara-Piyasekara
force-pushed
the
fix-error-code-inconsistency
branch
from
a96071c
to
26702f7
Compare
Thushara-Piyasekara
force-pushed
the
fix-error-code-inconsistency
branch
from
26702f7
to
3e017ec
Compare
...est/java/org/ballerinalang/test/types/readonly/TupleVsArrayReadonlyErrorConsistencyTest.java
Show resolved
Hide resolved
|
This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the |
...est/java/org/ballerinalang/test/types/readonly/TupleVsArrayReadonlyErrorConsistencyTest.java
Outdated
Show resolved
Hide resolved
|
This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the |
|
This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the |
|
Closed PR due to inactivity for more than 18 days. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #42521 +/- ##
==========================================
Coverage 77.67% 77.68%
- Complexity 51471 51474 +3
==========================================
Files 2933 2933
Lines 204124 204235 +111
Branches 26716 26716
==========================================
+ Hits 158559 158654 +95
- Misses 36938 36958 +20
+ Partials 8627 8623 -4 ☔ View full report in Codecov by Sentry. |
| public void testCreateDetails() { | ||
|
|
| * | ||
| * @since 2201.10.0 | ||
| */ | ||
| public class TupleVsArrayReadonlyErrorConsistencyTest { |
There was a problem hiding this comment.
Do we need to mention consistency here? I'd just include readonly field update.
There was a problem hiding this comment.
The "consistency" keyword was added because the issue being fixed by this PR #42520 is related to an inconsistency between the errors produced during runtime.
Are you suggesting to modify the test case to match the actual bug? or is it better to use the existing real world test case?
There was a problem hiding this comment.
I would just have them be part of readonly field tests. If this was the behaviour to start with, we wouldn't have had a consistency issue.
| @Test(expectedExceptions = BLangTestException.class, | ||
| expectedExceptionsMessageRegExp = "error: \\{ballerina/lang.map}InherentTypeViolation \\{\"message\":" + | ||
| "\"cannot update 'readonly' field 'name' in record of type '\\(Employee & readonly\\)'\".*") | ||
| public void testWithTupleUpdateMethod() { | ||
| BRunUtil.invoke(resultWithTupleUpdateMethod, "testFrozenAnyArrayElementUpdate"); | ||
| } |
There was a problem hiding this comment.
We can trap the panic and do the assertion in Ballerina itself, at least for new tests.
There was a problem hiding this comment.
Shall we use this as the new test and assert the exit code from java side?
public function main() returns error? {
error? actualError = trap testFrozenAnyArrayElementUpdate();
if actualError is error {
test:assertEquals(actualError.message(), "{ballerina/lang.map}InherentTypeViolation");
}
}There was a problem hiding this comment.
The test has to fail if actualError is not an error too.
|
This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the |
|
This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the |
Purpose
Fixes #42520
Approach
After a discussion with @heshanpadmasiri, it was apparent the
InherentTypeViolationerror forBFieldsof readonlyBRecordswere not getting triggered. This was due to the mutableBRecordgeneration not inheriting the readonly flag to the generatedBFields.After refactoring the logic to make the
BFieldsalso inherit the readonly flag, both cases mentioned in #42520 triggered the same error.Samples
Remarks
Check List