-
Notifications
You must be signed in to change notification settings - Fork 758
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Reset unknownTypeRefs
after silent type checking
#43758
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 8eed105.
...erina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java
Outdated
Show resolved
Hide resolved
@@ -9918,6 +9914,24 @@ String recordsToString(Set<BRecordType> recordTypeSet) { | |||
} | |||
} | |||
|
|||
public GlobalStateData getGlobalStateSnapshotAndResetGlobalState() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we do this via analyzer data instead of introducing a new data holder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AnalyzerData
act like set of global fields where we pass through the functions. GlobalStateData
is used as a snapshot of several fields where we will be creating multiple of objects of it. However, we can still use the AnalyzerData
for the same purpose. The issue is it will introduce unwanted object creations of the AnalyzerData
which is not clean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition is basically resetting unknown type refs, right? Can we just do it inline instead of introducing GlobalStateData
? The name is ambiguous and can be confusing with analyzer data IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing this inline is not a good idea in terms of scalability. We already have two fields that needs to be reset. There is a high chance of more fields require this in future. I have renamed the class name to GlobalStateSnapshot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only additional thing we've done is changing unknown type refs, right? Error count logic was already there? It makes sense to extract out repeated logic to a function, but IMO, here we are just doing it for part of it. I would rather refactor the whole thing properly or just reset type refs in line (similar to the others). With just what we have now, I also don't think GlobalStateSnapshot is an ideal name here, since it doesn't include all "global state" and can be confusing with analyzer data.
I would introduce the change in a way that makes it readable and clear at the moment, rather than anticipating future changes (when we can refactor everything properly).
...rina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeResolver.java
Outdated
Show resolved
Hide resolved
...erina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java
Show resolved
Hide resolved
@@ -9918,6 +9914,24 @@ String recordsToString(Set<BRecordType> recordTypeSet) { | |||
} | |||
} | |||
|
|||
public GlobalStateData getGlobalStateSnapshotAndResetGlobalState() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition is basically resetting unknown type refs, right? Can we just do it inline instead of introducing GlobalStateData
? The name is ambiguous and can be confusing with analyzer data IMO.
Purpose
$title
Fixes #43286
Approach
Compiler error at L#1749 was not reached because the field
unknownTypeRefs
was populated at the silent typechecking and does not reset after.ballerina-lang/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolResolver.java
Line 1749 in a6e4f99
Introduced
GlobalStateData
inner class to make the solution scalable for such fields may occur in futureSamples
The following sample should give unknown type error of unknown types
I
andJ
Check List