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.

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

Merged
merged 8 commits into from
Feb 10, 2025
Prev Previous commit
Next Next commit
Address review suggestions
rdulmina committed Jan 28, 2025
commit 4640af17467cc112e05a9f83fdeccfa57940153f
Original file line number Diff line number Diff line change
@@ -9916,9 +9916,7 @@ 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();
globalStateData.unknownTypeRefs = typeResolver.unknownTypeRefs;
globalStateData.errorCount = this.dlog.errorCount();
GlobalStateData globalStateData = new GlobalStateData(typeResolver.unknownTypeRefs, this.dlog.errorCount());

// Reset global state
typeResolver.unknownTypeRefs = new HashSet<>();
@@ -9951,8 +9949,6 @@ public static class AnalyzerData {
/**
rdulmina marked this conversation as resolved.
Show resolved Hide resolved
* @since 2201.12.0
*/
public static class GlobalStateData {
HashSet<TypeResolver.LocationData> unknownTypeRefs;
int errorCount;
public record GlobalStateData(HashSet<TypeResolver.LocationData> unknownTypeRefs, int errorCount) {
}
}