Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset unknownTypeRefs after silent type checking #43758

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+87 −36
Open

Reset unknownTypeRefs after silent type checking #43758

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8eed105
Add null check before accessing `type.tsymbol`
rdulmina Jan 6, 2025
87346d7
Merge remote-tracking branch 'upstream/master'
rdulmina Jan 7, 2025
a2b5709
Merge remote-tracking branch 'upstream/master'
rdulmina Jan 9, 2025
2558cd6
Revert "Add null check before accessing `type.tsymbol`"
rdulmina Jan 9, 2025
7264bbb
Merge remote-tracking branch 'upstream/master'
rdulmina Jan 21, 2025
65afcd1
Reset `unknownTypeRefs` after silent type checking
rdulmina Jan 21, 2025
4640af1
Address review suggestions
rdulmina Jan 28, 2025
9333af5
Address review suggestions
rdulmina Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...na-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolEnter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ private void defineConstructs(BLangPackage pkgNode, SymbolEnv pkgEnv) {
}
}
}
typeResolver.clearUnknowTypeRefs();
typeResolver.clearUnknownTypeRefs();
}

private void defineDependentFields(List<BLangNode> typeDefNodes, SymbolEnv pkgEnv) {
Expand Down
82 changes: 50 additions & 32 deletions ...na-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ public class TypeChecker extends SimpleBLangNodeAnalyzer<TypeChecker.AnalyzerDat
private final Types types;
private final Unifier unifier;
protected final QueryTypeChecker queryTypeChecker;
private final TypeResolver typeResolver;

static {
LIST_LENGTH_MODIFIER_FUNCTIONS.add(FUNCTION_NAME_PUSH);
Expand Down Expand Up @@ -324,6 +325,7 @@ public TypeChecker(CompilerContext context) {
this.missingNodesHelper = BLangMissingNodesHelper.getInstance(context);
this.unifier = new Unifier();
this.queryTypeChecker = QueryTypeChecker.getInstance(context);
this.typeResolver = TypeResolver.getInstance(context);
}

public TypeChecker(CompilerContext context, CompilerContext.Key<TypeChecker> key) {
Expand All @@ -343,6 +345,7 @@ public TypeChecker(CompilerContext context, CompilerContext.Key<TypeChecker> key
this.missingNodesHelper = BLangMissingNodesHelper.getInstance(context);
this.unifier = new Unifier();
this.queryTypeChecker = null;
this.typeResolver = TypeResolver.getInstance(context);
}

private BType checkExpr(BLangExpression expr, SymbolEnv env, AnalyzerData data) {
Expand Down Expand Up @@ -600,14 +603,13 @@ private BType silentIntTypeCheck(BLangLiteral literalExpr, Object literalValue,
AnalyzerData data) {
boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
data.commonAnalyzerData.nonErrorLoggingCheck = true;
int prevErrorCount = this.dlog.errorCount();
this.dlog.resetErrorCount();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();
this.dlog.mute();

BType exprCompatibleType = getIntegerLiteralType(nodeCloner.cloneNode(literalExpr), literalValue, expType,
data);
data.commonAnalyzerData.nonErrorLoggingCheck = prevNonErrorLoggingCheck;
this.dlog.setErrorCount(prevErrorCount);
restoreGlobalState(previousGlobalState);

if (!prevNonErrorLoggingCheck) {
this.dlog.unmute();
Expand Down Expand Up @@ -1224,7 +1226,7 @@ public void visit(BLangTableConstructorExpr tableConstructorExpr, AnalyzerData d

boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
data.commonAnalyzerData.nonErrorLoggingCheck = true;
int errorCount = this.dlog.errorCount();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();
this.dlog.mute();

List<BType> matchingTypes = new ArrayList<>();
Expand All @@ -1246,7 +1248,7 @@ public void visit(BLangTableConstructorExpr tableConstructorExpr, AnalyzerData d
}

data.commonAnalyzerData.nonErrorLoggingCheck = prevNonErrorLoggingCheck;
this.dlog.setErrorCount(errorCount);
restoreGlobalState(previousGlobalState);
if (!prevNonErrorLoggingCheck) {
this.dlog.unmute();
}
Expand Down Expand Up @@ -1803,7 +1805,7 @@ private BType checkListConstructorCompatibility(BType referredType, BType origin
int tag = referredType.tag;
if (tag == TypeTags.UNION) {
boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
int errorCount = this.dlog.errorCount();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();
data.commonAnalyzerData.nonErrorLoggingCheck = true;
this.dlog.mute();

Expand Down Expand Up @@ -1832,7 +1834,7 @@ private BType checkListConstructorCompatibility(BType referredType, BType origin
}

data.commonAnalyzerData.nonErrorLoggingCheck = prevNonErrorLoggingCheck;
this.dlog.setErrorCount(errorCount);
restoreGlobalState(previousGlobalState);
if (!prevNonErrorLoggingCheck) {
this.dlog.unmute();
}
Expand Down Expand Up @@ -2545,7 +2547,7 @@ public BType checkMappingConstructorCompatibility(BType bType, BLangRecordLitera
if (tag == TypeTags.UNION) {
boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
data.commonAnalyzerData.nonErrorLoggingCheck = true;
int errorCount = this.dlog.errorCount();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();
this.dlog.mute();

List<BType> compatibleTypes = new ArrayList<>();
Expand Down Expand Up @@ -2574,7 +2576,7 @@ public BType checkMappingConstructorCompatibility(BType bType, BLangRecordLitera
}

data.commonAnalyzerData.nonErrorLoggingCheck = prevNonErrorLoggingCheck;
dlog.setErrorCount(errorCount);
restoreGlobalState(previousGlobalState);
if (!prevNonErrorLoggingCheck) {
this.dlog.unmute();
}
Expand Down Expand Up @@ -3928,13 +3930,13 @@ private void validateErrorConstructorPositionalArgs(BLangErrorConstructorExpr er
protected BType checkExprSilent(BLangExpression expr, BType expType, AnalyzerData data) {
boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
data.commonAnalyzerData.nonErrorLoggingCheck = true;
int errorCount = this.dlog.errorCount();
this.dlog.mute();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();

BType type = checkExpr(expr, expType, data);

data.commonAnalyzerData.nonErrorLoggingCheck = prevNonErrorLoggingCheck;
dlog.setErrorCount(errorCount);
restoreGlobalState(previousGlobalState);
if (!prevNonErrorLoggingCheck) {
this.dlog.unmute();
}
Expand Down Expand Up @@ -5469,15 +5471,14 @@ public boolean isOptionalFloatOrDecimal(BType expectedType) {
private BType checkAndGetType(BLangExpression expr, SymbolEnv env, BLangBinaryExpr binaryExpr, AnalyzerData data) {
boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
data.commonAnalyzerData.nonErrorLoggingCheck = true;
int prevErrorCount = this.dlog.errorCount();
this.dlog.resetErrorCount();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();
this.dlog.mute();

expr.cloneAttempt++;
BType exprCompatibleType = checkExpr(nodeCloner.cloneNode(expr), env, binaryExpr.expectedType, data);
data.commonAnalyzerData.nonErrorLoggingCheck = prevNonErrorLoggingCheck;
int errorCount = this.dlog.errorCount();
this.dlog.setErrorCount(prevErrorCount);
restoreGlobalState(previousGlobalState);
if (!prevNonErrorLoggingCheck) {
this.dlog.unmute();
}
Expand Down Expand Up @@ -5731,26 +5732,25 @@ public boolean silentCompatibleFiniteMembersInUnionTypeCheck(BLangUnaryExpr unar
AnalyzerData data) {
boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
data.commonAnalyzerData.nonErrorLoggingCheck = true;
int prevErrorCount = this.dlog.errorCount();
this.dlog.resetErrorCount();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();
this.dlog.mute();

BType compatibleTypeOfUnaryExpression;
for (BType type : expectedType.getMemberTypes()) {
compatibleTypeOfUnaryExpression = checkExpr(nodeCloner.cloneNode(unaryExpr), Types.getImpliedType(type),
data);
if (Types.getImpliedType(compatibleTypeOfUnaryExpression).tag == TypeTags.FINITE) {
unmuteDlog(data, prevNonErrorLoggingCheck, prevErrorCount);
unmuteDlog(data, prevNonErrorLoggingCheck, previousGlobalState);
return true;
}
}
unmuteDlog(data, prevNonErrorLoggingCheck, prevErrorCount);
unmuteDlog(data, prevNonErrorLoggingCheck, previousGlobalState);
return false;
}

private void unmuteDlog(AnalyzerData data, boolean prevNonErrorLoggingCheck, int prevErrorCount) {
private void unmuteDlog(AnalyzerData data, boolean prevNonErrorLoggingCheck, GlobalStateData previousGlobalState) {
data.commonAnalyzerData.nonErrorLoggingCheck = prevNonErrorLoggingCheck;
this.dlog.setErrorCount(prevErrorCount);
restoreGlobalState(previousGlobalState);
if (!prevNonErrorLoggingCheck) {
this.dlog.unmute();
}
Expand All @@ -5759,13 +5759,12 @@ private void unmuteDlog(AnalyzerData data, boolean prevNonErrorLoggingCheck, int
public BType silentTypeCheckExpr(BLangExpression expr, BType referredType, AnalyzerData data) {
boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
data.commonAnalyzerData.nonErrorLoggingCheck = true;
int prevErrorCount = this.dlog.errorCount();
this.dlog.resetErrorCount();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();
this.dlog.mute();

BType exprCompatibleType = checkExpr(nodeCloner.cloneNode(expr), referredType, data);

unmuteDlog(data, prevNonErrorLoggingCheck, prevErrorCount);
unmuteDlog(data, prevNonErrorLoggingCheck, previousGlobalState);
return exprCompatibleType;
}

Expand Down Expand Up @@ -5823,14 +5822,13 @@ public void visit(BLangTypeConversionExpr conversionExpr, AnalyzerData data) {

boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
data.commonAnalyzerData.nonErrorLoggingCheck = true;
int prevErrorCount = this.dlog.errorCount();
this.dlog.resetErrorCount();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();
this.dlog.mute();

BType exprCompatibleType = checkExpr(nodeCloner.cloneNode(expr), targetType, data);
data.commonAnalyzerData.nonErrorLoggingCheck = prevNonErrorLoggingCheck;
int errorCount = this.dlog.errorCount();
this.dlog.setErrorCount(prevErrorCount);
restoreGlobalState(previousGlobalState);

if (!prevNonErrorLoggingCheck) {
this.dlog.unmute();
Expand Down Expand Up @@ -6609,9 +6607,8 @@ private BType getCandidateLaxType(BLangNode expr, BType rhsType) {
private BType getCandidateType(BLangCheckedExpr checkedExpr, BType checkExprCandidateType, AnalyzerData data) {
boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
data.commonAnalyzerData.nonErrorLoggingCheck = true;
int prevErrorCount = this.dlog.errorCount();
this.dlog.resetErrorCount();
this.dlog.mute();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();

checkedExpr.expr.cloneAttempt++;
BLangExpression clone = nodeCloner.cloneNode(checkedExpr.expr);
Expand All @@ -6622,7 +6619,7 @@ private BType getCandidateType(BLangCheckedExpr checkedExpr, BType checkExprCand
rhsType = checkExpr(clone, checkExprCandidateType, data);
}
data.commonAnalyzerData.nonErrorLoggingCheck = prevNonErrorLoggingCheck;
this.dlog.setErrorCount(prevErrorCount);
restoreGlobalState(previousGlobalState);
if (!prevNonErrorLoggingCheck) {
this.dlog.unmute();
}
Expand Down Expand Up @@ -8243,15 +8240,14 @@ private List<BLangExpression> concatSimilarKindXMLNodes(List<BLangExpression> ex
for (BLangExpression expr : exprs) {
boolean prevNonErrorLoggingCheck = data.commonAnalyzerData.nonErrorLoggingCheck;
data.commonAnalyzerData.nonErrorLoggingCheck = true;
int prevErrorCount = this.dlog.errorCount();
this.dlog.resetErrorCount();
GlobalStateData previousGlobalState = getGlobalStateSnapshotAndResetGlobalState();
this.dlog.mute();

BType exprType = checkExpr(nodeCloner.cloneNode(expr), xmlElementEnv, symTable.xmlType, data);

data.commonAnalyzerData.nonErrorLoggingCheck = prevNonErrorLoggingCheck;
int errorCount = this.dlog.errorCount();
this.dlog.setErrorCount(prevErrorCount);
restoreGlobalState(previousGlobalState);

if (!prevNonErrorLoggingCheck) {
this.dlog.unmute();
Expand Down Expand Up @@ -9918,6 +9914,22 @@ String recordsToString(Set<BRecordType> recordTypeSet) {
}
}

public GlobalStateData getGlobalStateSnapshotAndResetGlobalState() {
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this approach is less readable than the inline one. If we write this inline the code becomes messy, not scalable, and the same code will duplicate at every place where we do silent type checking.

It makes sense to extract out repeated logic to a function

Yes, to extract out repeated logic we need some kind of a object or a record to get the previous data available at the caller function

// Preserve global state
GlobalStateData globalStateData = new GlobalStateData(typeResolver.unknownTypeRefs, this.dlog.errorCount());

// Reset global state
typeResolver.unknownTypeRefs = new HashSet<>();
this.dlog.resetErrorCount();

return globalStateData;
}

public void restoreGlobalState(GlobalStateData globalStateData) {
typeResolver.unknownTypeRefs = globalStateData.unknownTypeRefs;
this.dlog.setErrorCount(globalStateData.errorCount);
}

/**
* @since 2.0.0
*/
Expand All @@ -9933,4 +9945,10 @@ public static class AnalyzerData {
QueryTypeChecker.AnalyzerData queryData = new QueryTypeChecker.AnalyzerData();
Set<String> queryVariables;
}

/**
rdulmina marked this conversation as resolved.
Show resolved Hide resolved
* @since 2201.12.0
*/
public record GlobalStateData(HashSet<TypeResolver.LocationData> unknownTypeRefs, int errorCount) {
}
}
6 changes: 3 additions & 3 deletions ...a-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public class TypeResolver {
private final HashSet<BLangClassDefinition> resolvedClassDef = new HashSet<>();
private final Map<String, BLangNode> modTable = new LinkedHashMap<>();
private final Map<String, BLangConstantValue> constantMap = new HashMap<>();
private final HashSet<LocationData> unknownTypeRefs;
public HashSet<LocationData> unknownTypeRefs;
rdulmina marked this conversation as resolved.
Show resolved Hide resolved
private SymbolEnv pkgEnv;
private int currentDepth;
private Deque<BType> resolvingTypes;
Expand All @@ -188,7 +188,7 @@ public TypeResolver(CompilerContext context) {
this.unknownTypeRefs = new HashSet<>();
}

public void clearUnknowTypeRefs() {
public void clearUnknownTypeRefs() {
unknownTypeRefs.clear();
}

Expand Down Expand Up @@ -2133,7 +2133,7 @@ private static class ResolverData {
*
* @since 2201.7.0
*/
private static class LocationData {
protected static class LocationData {
private final String name;
private final int row;
private final int column;
Expand Down
6 changes: 6 additions & 0 deletions ...-unit-test/src/test/java/org/ballerinalang/test/typedefs/TypeDefinitionsNegativeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ public void testSemanticsNegative() {
"'function () returns (int)', found 'function () returns (MyTuple)'", 37, 33);
BAssertUtil.validateError(compileResult, index++, "incompatible types: expected 'Foo', found 'string'", 43, 13);

BAssertUtil.validateError(compileResult, index++, "unknown type 'I'", 48, 19);
BAssertUtil.validateError(compileResult, index++, "unknown type 'I'", 52, 35);
BAssertUtil.validateError(compileResult, index++, "unknown type 'J'", 54, 17);
BAssertUtil.validateError(compileResult, index++, "unknown type 'J'", 56, 22);
BAssertUtil.validateError(compileResult, index++, "unknown type 'J'", 58, 23);

Assert.assertEquals(compileResult.getErrorCount(), index);
}
}
14 changes: 14 additions & 0 deletions ...na-unit-test/src/test/resources/test-src/typedefs/type-definitions-semantics-negative.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,17 @@ type Foo boolean|null;
function testNullFiniteType() {
Foo _ = "null"; // error
}

class H {};

H res = check new I();

int[] a = [1, 2, 3, 4, 5];

int[] b = from var i in a select <I> i;

float result = <J>1 + 2.0;

int result2 = true? <J>1 : 2;

var result3 = <H> new J();
Loading