Add the locations to the field in an intersected record #42108
Conversation
nipunayf
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #42108 +/- ##
=========================================
Coverage 76.69% 76.69%
Complexity 53036 53036
=========================================
Files 2883 2883
Lines 199951 199958 +7
Branches 26034 26026 -8
=========================================
+ Hits 153345 153353 +8
- Misses 38144 38146 +2
+ Partials 8462 8459 -3 ☔ View full report in Codecov by Sentry. |
nipunayf
requested review from
MaryamZi,
KavinduZoysa,
LakshanWeerasinghe,
mindula and
chiranSachintha
nipunayf
added
the
Team/CompilerFE
...s/ballerina-compiler-api-test/src/test/resources/test-src/symbols/error_type_symbol_test.bal
Show resolved
Hide resolved
|
@MaryamZi, Do we need to send this fix to 8.x branch? |
| @@ -5618,7 +5618,7 @@ private boolean populateFields(IntersectionContext intersectionContext, BRecordT | |||
| intersectionFieldType, newTypeSymbol, lhsRecordField.pos, SOURCE); | |||
| } | |||
|
|
|||
| newTypeFields.put(key, new BField(name, null, recordFieldSymbol)); | |||
| newTypeFields.put(key, new BField(name, recordFieldSymbol.pos, recordFieldSymbol)); | |||
There was a problem hiding this comment.
It is okay to set the position here. My concern is why we keep the position as a field of BField. Normally, we access the position from the symbol. Here as well, when we need to retrieve the position, we need to obtain it from the symbol.
There was a problem hiding this comment.
When we create the record field symbol of the resultant record symbol of the intersection, we use the position of this BField. Alternatively, we can use lhsRecordField.symbol.pos as well here.
There was a problem hiding this comment.
Yes. IMO, That is the best way to access the position. Shall we try whether we can remove pos in the BField without affecting the code?
nipunayf
dismissed stale reviews from KavinduZoysa and LakshanWeerasinghe
via
ee57954
nipunayf
requested review from
chiranSachintha,
KavinduZoysa and
LakshanWeerasinghe
| } | ||
|
|
||
| newTypeFields.put(key, new BField(name, recordFieldSymbol.pos, recordFieldSymbol)); | ||
| newTypeFields.put(key, new BField(name, null, recordFieldSymbol)); |
There was a problem hiding this comment.
I think you got what I said wrong. What I meant was, check whether we can remove the pos field from the BField. If we can't, then let's just use recordFieldSymbol.pos for pos here.
There was a problem hiding this comment.
Currently, there are multiple usages that use the pos field of the BField. We may require a separate issue to evaluate and refactor this change if applicable.
KavinduZoysa
merged commit 0823dc9
into
ballerina-platform:master
Purpose
The current implementation sets
nullas the location when adding the fields to the new intersected record. This impacts thefieldDescriptorsAPI as it is unable to detect the line range when initializing the symbols.Fixes #42094
Check List