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

[Improvement]: Integrate subset of semtypes into jbllerina compile-time #40045

Closed
lochana-chathura opened this issue Mar 30, 2023 · 9 comments · Fixed by #40771
Closed

[Improvement]: Integrate subset of semtypes into jbllerina compile-time #40045

lochana-chathura opened this issue Mar 30, 2023 · 9 comments · Fixed by #40771
Assignees
Labels
Area/SemtypeIntegration Issue related to integrating semtype engine Points/3 Equivalent to three days effort Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. Type/Improvement

Comments

@lochana-chathura
Copy link
Member

lochana-chathura commented Mar 30, 2023

Description

$subject.

Under this following sem-types are implemented.

  • int
  • boolean
  • float
  • decimal
  • string
  • nil
  • never

Union and Intersection will not be supported. However, finite-type unions will be supported. e.g. "x"|"y".

We will use the resolved sem type for subtyping checks in Types.isAssignable().

@lochana-chathura lochana-chathura added Type/Improvement Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. labels Mar 30, 2023
@lochana-chathura lochana-chathura added the Points/3 Equivalent to three days effort label Mar 30, 2023
@lochana-chathura lochana-chathura self-assigned this Apr 3, 2023
@lochana-chathura
Copy link
Member Author

We are keeping an additional property semtype in BType class and we resolve a semtype additionally while type resolving which will then be used to check if one type is a subtype of the other. W.R.T the current jBallerina implementation, our first goal is to replace the current isAssignable() and isSameType() function usages to use the new SemTypes.isSubtype() where the subtyping is determined based on the resolved semtypes.

@lochana-chathura
Copy link
Member Author

lochana-chathura commented Apr 17, 2023

At the movement, the ported semtype logic in nutcracker branch is outdated especially record and table types. Also, the object type is not supported. Therefore, we will limit the SemTypes.isSubtype() usage for simple types like int, string, boolean, etc initially. The exact set will be updated later.

Currently, semtype resolving is done for all the declared type definitions and they are used for subtyping.
e.g.

Type Three 3;

Type Int int;

public function main() {
   Three a = 3;
   Int b = a; // [1]
}

In above sample at [1], for assignability we will now use the new SemTypes.isSubtype().

However, when the types are not declared via a type definition we are not resolving the semtypes and thus are not used for subtyping.
e.g.

public function main() {
   3 a = 3;
   int b = a;
}

Further, If at least one type does not have a resolved semtype, we will be using the old jballerina isAssignable() logic. We have to do this way until we completely support all the types in the semtype engine.

@lochana-chathura
Copy link
Member Author

When implementing logic for the second path in the above description. Ran into a langlib failure.

Error building Ballerina package: lang.stream
ERROR [stream.bal:(129:25,129:39)] invalid operation: type 'never' does not support optional field access
ERROR [stream.bal:(160:25,160:38)] invalid operation: type 'never' does not support field access

Currently looking at it.

@lochana-chathura
Copy link
Member Author

Currently looking at it.

The reason for the failure seem to be caused at,

remainingTypes.removeIf(type -> isAssignable(type, removeType));

Here, the new SemTypes.isSubtype() seem to give a wrong result for two types error and record {| ballerina/lang.stream:0.0.0:Type value; |}.

Screenshot from 2023-04-18 18-32-46

@lochana-chathura
Copy link
Member Author

Here, the new SemTypes.isSubtype() seem to give a wrong result for two types error and record {| ballerina/lang.stream:0.0.0:Type value; |}.

The resolved semType for record {| ballerina/lang.stream:0.0.0:Type value; |} is a recursive semtype which is wrong. The reason is we try to resolve the same semType twice for the above record and on the first attempt we fail. But, we don't clean up the td.defn = new MappingDefinition() so in the next attempt,

  if (td.defn != null) {
            return td.defn.getSemType(semtypeEnv);
  }

will end up creating a recursive semType for the record.

@lochana-chathura
Copy link
Member Author

As suggested by @hasithaa we will have the sem type-based type checking disabled by default. (the reason is, though we initially support simple types like int, boolean, string, etc which are less likely to break with the new type engine, once we extend it with types like records, tuples, etc we may face unforeseen issues.)

We will have a CLI option to enable the sem-type engine (e.g. bal build/run --semtype-enabled). Also suggested having a daily build with unit tests running on top of the sem-type engine.

@lochana-chathura
Copy link
Member Author

Progress Update:
Currently, sem-types are supported for the following types:

    private boolean isSemTypeEnabled(BType bType) {
        switch (bType.tag) {
            case TypeTags.NEVER:
            case TypeTags.NIL:
            case TypeTags.BOOLEAN:
            case TypeTags.FLOAT:
            case TypeTags.DECIMAL:
            case TypeTags.STRING:
            case TypeTags.CHAR_STRING:
            case TypeTags.INT:
            case TypeTags.BYTE:
            case TypeTags.SIGNED8_INT:
            case TypeTags.SIGNED16_INT:
            case TypeTags.SIGNED32_INT:
            case TypeTags.UNSIGNED8_INT:
            case TypeTags.UNSIGNED16_INT:
            case TypeTags.UNSIGNED32_INT:
            case TypeTags.FINITE:
                return true;
            case TypeTags.TYPEREFDESC:
                return isSemTypeEnabled(getReferredType(bType));
            default:
                return false;
        }
    }

However, BIRTypeWriter and BIRPackageSimbolEnter are not supported for the resolved SemTypes. Right now it is in progress.

@lochana-chathura
Copy link
Member Author

lochana-chathura commented May 17, 2023

Currently, :bir-spec:test is failing due to not updating the bir.ksy for the new bir sem type changes.
Changes to bir. ksy is in progress. Other than that unit tests are passing for the expected scenarios.

@lochana-chathura lochana-chathura changed the title [Improvement]: Integrate semtype engine into jbllerina [Improvement]: Integrate subset of semtypes into jbllerina compile-time May 19, 2023
@lochana-chathura
Copy link
Member Author

Closing with #40300. Can track changes being merged into the master via the parent issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/SemtypeIntegration Issue related to integrating semtype engine Points/3 Equivalent to three days effort Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. Type/Improvement
Projects
Archived in project
1 participant