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

Revamp SymbolEnter to properly resolve constants #36538

Closed
SandaruJayawardana opened this issue Jun 15, 2022 · 9 comments · Fixed by #40663
Closed

Revamp SymbolEnter to properly resolve constants #36538

SandaruJayawardana opened this issue Jun 15, 2022 · 9 comments · Fixed by #40663
Assignees
Labels
Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. Type/Improvement
Milestone

Comments

@SandaruJayawardana
Copy link
Contributor

SandaruJayawardana commented Jun 15, 2022

Description:
Related discussion - #35745

It is necessary to properly type check and resolve constants at the earliest possible stage, since constants may be required to resolve other variables, type defn etc.. According to the spec, constants are known at compile time and they can be only dependent on other constants or type definitions which are also known at compile time. Therefore, we can resolve constants at the SymbolEnter phase.

Describe your problem(s)
In the current implementation, compiler resolves the constants at ConstantValueResolver in semantic analyzer phase. But type nodes of module level variables are already resolved before resolving the constants. Therefore, even compiler updates the constants with correct types in later stage, it will not fix the module level usages (as a type) of the constants.

Describe your solution(s)
Will update once the approach is finalized.

Related Issues (optional):
#28334, #32804, #33890

@SandaruJayawardana SandaruJayawardana added Type/Improvement Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. labels Jun 15, 2022
@SandaruJayawardana SandaruJayawardana changed the title Revamp SymboleEnter to properly resolve constants Revamp SymbolEnter to properly resolve constants Jun 15, 2022
@SandaruJayawardana SandaruJayawardana self-assigned this Jun 29, 2022
@KavinduZoysa KavinduZoysa added the Points/5 Equivalent to five day effort label Jun 29, 2022
@gimantha gimantha added this to the 2201.3.0 milestone Jul 14, 2022
@ushirask ushirask added Points/8 Equivalent to eight day effort and removed Points/5 Equivalent to five day effort labels Jul 14, 2022
@MaryamZi MaryamZi added Points/5 Equivalent to five day effort and removed Points/8 Equivalent to eight day effort labels Aug 10, 2022
@chiranSachintha chiranSachintha added Points/4 Equivalent to four day effort and removed Points/5 Equivalent to five day effort labels Aug 24, 2022
@gimantha gimantha added Points/8 Equivalent to eight day effort and removed Points/4 Equivalent to four day effort labels Sep 7, 2022
@prakanth97
Copy link
Contributor

Initially, there were 560 failed tests and approximately 1500 tests that were skipped. By addressing the unimplemented cases in the type resolver and constant type checker, we were able to reduce the number of failing unit tests to 65.

@prakanth97
Copy link
Contributor

In the current constant type checker, when evaluating the const-expr the expected type of the module-const-decl is not considered. Due to that it violates below case.

// According the spec, 
// type of `1` is float and type of `2.0` is float. hence it is valid.
const float|decimal X1 = 1 + 2.0;

// type of `2.0` is float but type of `3.0d` is decimal. hence it is not valid.
const float|decimal X2 = 2.0 + 3.0d;

// type of `2` is float but type of `2.0d` is decimal. hence it is not valid.
const float|decimal Z2 = 2 * 2.0d;

But according to the current implementation, all are valid cases. We are currently updating the constant type checker to handle these scenarios properly.

@prakanth97
Copy link
Contributor

In the current constant type checker, when evaluating the const-expr the expected type of the module-const-decl is not considered. Due to that it violates below case.

// According the spec, 
// type of `1` is float and type of `2.0` is float. hence it is valid.
const float|decimal X1 = 1 + 2.0;

// type of `2.0` is float but type of `3.0d` is decimal. hence it is not valid.
const float|decimal X2 = 2.0 + 3.0d;

// type of `2` is float but type of `2.0d` is decimal. hence it is not valid.
const float|decimal Z2 = 2 * 2.0d;

But according to the current implementation, all are valid cases. We are currently updating the constant type checker to handle these scenarios properly.

This is fixed by rewriting the constant type checker to use expected type to determine the literal types instead of calculating all possible candidates.

@hasithaa hasithaa removed the Points/8 Equivalent to eight day effort label Mar 16, 2023
@prakanth97
Copy link
Contributor

To allow generic intersection we have to resolve effective type of intersection type definition using depth first approach. But there is a limitation when we have cyclic reference in the type definition.

type A object {
   B b;
   int a;
};

type B A & readonly;

When resolving B, type A is not resolved completely. So immutable clone will have the partially defined type. To solve this, first we have to calculate partially defined effective type for B. After fully defined the A we will visit type of B and update the effective type.

@prakanth97
Copy link
Contributor

  • All the existing langlib tests and unit-tests have passed.
  • If the type definition is readonly intersection with inherently immutable type then we set the inherently immutable type as type of type definition to optimise the runtime.

Currently working on updating immutable type (effective type of readonly intersection) of tuple type.

type A [B];
type B A & readonly;
. . . . 
type A B & readonly;
type B [A]|int;
. . . .
type A [B] & readonly;
type B A|int;
. . . .
type A [B] & readonly;
type B [A]|int;

These are the few cyclic tuple case, where we have to update the improperly defined effective type.

I'm getting below runtime error for

type A [B] & readonly;
type B A|int;

Currently looking into why is it failing

java.lang.NoSuchFieldError: $type$A
                    at constants.$_typeref_type_constants.$populate$typeRefType$A(Unknown Source)
                    at constants.$_typeref_type_constants.$populate_typeref_types(Unknown Source)
                    at types.$_types.$createTypeConstants(Unknown Source)
                    at types.$_types.$createTypes(Unknown Source)
                    at $_init.$currentModuleInit(.)
                    at $configurationMapper.$initAndPopulateConfigData(Unknown Source)
                    at $configurationMapper.$configureInit(Unknown Source)

@prakanth97
Copy link
Contributor

prakanth97 commented Apr 21, 2023

To allow generic intersection we have to resolve effective type of intersection type definition using depth first approach. But there is a limitation when we have cyclic reference in the type definition.

type A object {
   B b;
   int a;
};

type B A & readonly;

When resolving B, type A is not resolved completely. So immutable clone will have the partially defined type. To solve this, first we have to calculate partially defined effective type for B. After fully defined the A we will visit type of B and update the effective type.

  • Record type, object type, union type are handled by implementing visitors to update immutable type.
  • Related tests have passed.
  • Need to implement visitors for other BTypes.

@prakanth97
Copy link
Contributor

type C record {
    *B;
};

type A record {|
    B b;
    int a;
|};

type B A & readonly;

inclusion of cyclic record is not handled. We consider it as know issue and attend that later

@prakanth97
Copy link
Contributor

For this case we are logging a compile-time error invalid usage of finite literal: duplicate key 'a'.

const x = "a";
const map<string> X = {a : "A", [x] : "B"};

@prakanth97
Copy link
Contributor

class A {
    *B;
}

class B {
    *A;
}

This should result invalid cyclic reference error like record and object type definitions. Currently fixing it

@prakanth97 prakanth97 added this to the 2201.7.0 milestone Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. Type/Improvement
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants