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

Introduce new isEnum() and getEnumSymbol() APIs in UnionTypeSymbol to distinguish and get enum-symbol in union-type descriptors #41173

Closed
wants to merge 5 commits into from
Closed

Introduce new isEnum() and getEnumSymbol() APIs in UnionTypeSymbol to distinguish and get enum-symbol in union-type descriptors #41173

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ceee36a
Introduce isEnum and getEnumSymbol APIs
dulajdilshan Aug 8, 2023
bb912eb
Add tests
dulajdilshan Aug 8, 2023
7a1e398
Remove unnecessary field
dulajdilshan Aug 8, 2023
b75fc71
Merge branch 'master' into fix/enum-symbol
dulajdilshan Aug 10, 2023
41b4726
Improve tests and address a review suggestion
dulajdilshan Aug 10, 2023
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
33 changes: 33 additions & 0 deletions ...a-lang/src/main/java/io/ballerina/compiler/api/impl/symbols/BallerinaUnionTypeSymbol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,15 @@
import io.ballerina.compiler.api.ModuleID;
import io.ballerina.compiler.api.SymbolTransformer;
import io.ballerina.compiler.api.SymbolVisitor;
import io.ballerina.compiler.api.impl.SymbolFactory;
import io.ballerina.compiler.api.symbols.EnumSymbol;
import io.ballerina.compiler.api.symbols.TypeDescKind;
import io.ballerina.compiler.api.symbols.TypeSymbol;
import io.ballerina.compiler.api.symbols.UnionTypeSymbol;
import org.ballerinalang.model.symbols.SymbolKind;
import org.ballerinalang.model.types.TypeKind;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BEnumSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BTypeSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.Symbols;
import org.wso2.ballerinalang.compiler.semantics.model.types.BFiniteType;
import org.wso2.ballerinalang.compiler.semantics.model.types.BType;
Expand All @@ -36,6 +41,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.StringJoiner;
import java.util.regex.Pattern;
Expand All @@ -58,6 +64,7 @@ public class BallerinaUnionTypeSymbol extends AbstractTypeSymbol implements Unio
private List<TypeSymbol> memberTypes;
private List<TypeSymbol> originalMemberTypes;
private String signature;
private EnumSymbol enumSymbol;

public BallerinaUnionTypeSymbol(CompilerContext context, BUnionType unionType) {
super(context, TypeDescKind.UNION, unionType);
Expand Down Expand Up @@ -147,6 +154,32 @@ public String signature() {
return this.signature;
}

@Override
public boolean isEnum() {
if (this.enumSymbol != null) {
return true;
}

return this.getBType().tsymbol.getKind() == SymbolKind.ENUM;
}

@Override
public Optional<EnumSymbol> getEnumSymbol() {
Comment on lines +166 to +167
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is ideal. isEnum kinda makes sense, but why do we need to allow retrieving the enum symbol like this? In a way, it's like having a reference to the definition from the type symbol.

Copy link
Contributor Author

@dulajdilshan dulajdilshan Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. It's kinda exposing the information related to the enum symbol. Currently, we may not have such things, but having this may allow us to expose that information right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and this is not a way of referencing. For referencing we have a structure as TypeRefType

Copy link
Member

Choose a reason for hiding this comment

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

I meant having enumSymbol in BallerinaUnionTypeSymbol. This isn't quite right, IMO. The type doesn't need to how it was defined.

Yes. It's kinda exposing the information related to the enum symbol. Currently, we may not have such things, but having this may allow us to expose that information right?

I don't think that's the correct way. If someone wants enum symbol they can still get it from the model, right? I don't think the type symbol should expose it.

Copy link
Contributor Author

@dulajdilshan dulajdilshan Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I meant having enumSymbol in BallerinaUnionTypeSymbol. This isn't quite right, IMO. The type doesn't need to how it was defined.

yeah. What if we have introduced some attributes only related to enums and the ballerina extension developer needs to get them? We can't expose that only-enum information via the BallerinaUnionTypeSymbol right?

I don't think that's the correct way. If someone wants enum symbol they can still get it from the model, right? I don't think the type symbol should expose it.

How can they get an enum symbol without the location?

Copy link
Member

Choose a reason for hiding this comment

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

I meant having enumSymbol in BallerinaUnionTypeSymbol. This isn't quite right, IMO. The type doesn't need to how it was defined.

yeah. What if we have introduced some attributes only related to enums and the ballerina extension developer needs to get them? We can't expose that only-enum information via the BallerinaUnionTypeSymbol right?

Can you give an example for this?

I don't think that's the correct way. If someone wants enum symbol they can still get it from the model, right? I don't think the type symbol should expose it.

How can they get an enum symbol without the location?

Can't they filter out from module symbols by name?

Copy link
Member

Choose a reason for hiding this comment

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

We can't expose that only-enum information via the BallerinaUnionTypeSymbol right?

Yes, we can't. I can't think of such info anyway. We shouldn't mix up enums and types. An enum is just another way to define a union of string singletons. From a type symbol point of view, they are basically the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give an example for this?

what about annotation attachments? We shouldn't link them with the UnionTypeSymbol right?

Can't they filter out module symbols by name?

Yeah, we can, but we need the name to filter out right? What if we need to get the enum that is in another module, and we have referenced an enum value (e.x: OPEN) and we need to know the other sibling values of the enum (which is referenced in new type?

//  defined in another module `test0.module1`'
public enum State {
    OPEN,
    CLOSED
}

// main.bal
import test0.module1;

type ConnectionState State;

function test() {
    string x = module1.OPEN;
}

Let's say there is a ballerina extension writer who needs to get only the sibling values of OPEN.

Also, isn't it costly to use the filter when there are so many symbols?

Yes, we can't. I can't think of such info anyway. We shouldn't mix up enums and types. An enum is just another way to define a union of string singletons. From a type symbol point of view, they are basically the same.

Yes. This is true, but here we are just showing the relationship with the associated enum defined somewhere

Copy link
Member

Choose a reason for hiding this comment

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

what about annotation attachments? We shouldn't link them with the UnionTypeSymbol right?

annotation annot on type;

const RED = "RED";
const BLUE = "BLUE";

@annot
type Colour RED|BLUE;

is the same as

annotation annot on type;

@annot
enum Colour {
    RED,
    BLUE
}

so we shouldn't have to special case anything for enums.

Yeah, we can, but we need the name to filter out right?

If you have the union type you have the name?

What if we need to get the enum that is in another module, and we have referenced an enum value (e.x: OPEN) and we need to know the other sibling values of the enum (which is referenced in new type?

How does adding the enum symbol to the union type help with this? The union type corresponds to the enum, not the member?

Also, isn't it costly to use the filter when there are so many symbols?

It's just filtering based on a string? Anyway, the API being well-defined is more important IMO.

if (this.enumSymbol != null) {
return Optional.of(this.enumSymbol);
}

BTypeSymbol tsymbol = this.getBType().tsymbol;
if (tsymbol.getKind() != SymbolKind.ENUM) {
return Optional.empty();
}

SymbolFactory symbolFactory = SymbolFactory.getInstance(this.context);
this.enumSymbol = symbolFactory.createEnumSymbol((BEnumSymbol) tsymbol, tsymbol.getName().value);;

return Optional.ofNullable(this.enumSymbol);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return Optional.ofNullable(this.enumSymbol);
return Optional.of(this.enumSymbol);

At this point can the enumSymbol be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing!

}

@Override
public void accept(SymbolVisitor visitor) {
visitor.visit(this);
Expand Down
15 changes: 15 additions & 0 deletions compiler/ballerina-lang/src/main/java/io/ballerina/compiler/api/symbols/UnionTypeSymbol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package io.ballerina.compiler.api.symbols;

import java.util.List;
import java.util.Optional;

/**
* Represents an union type descriptor.
Expand All @@ -39,4 +40,18 @@ public interface UnionTypeSymbol extends TypeSymbol {
* @return {@link List} of expanded member types
*/
List<TypeSymbol> memberTypeDescriptors();

/**
* Check whether the union type is an enum.
*
* @return {@code true} if the union type is an enum, {@code false} otherwise
*/
boolean isEnum();

/**
* Get the enum symbol if the union type is an enum.
*
* @return Optional of {@link EnumSymbol} enum symbol
*/
Optional<EnumSymbol> getEnumSymbol();
}
27 changes: 27 additions & 0 deletions ...er-api-test/src/test/java/io/ballerina/semantic/api/test/symbols/UnionTypeSymbolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import io.ballerina.compiler.api.symbols.UnionTypeSymbol;
import io.ballerina.projects.Document;
import io.ballerina.projects.Project;
import io.ballerina.semantic.api.test.util.SemanticAPITestUtils;
import org.ballerinalang.test.BCompileUtil;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.DataProvider;
Expand Down Expand Up @@ -116,6 +117,32 @@ public void testSingletonMembers() {
assertList(signatures, List.of("\"int\"", "\"string\"", "100", "\"200\"", "true"));
}

@Test(dataProvider = "DataForEnumInUnionTypeSymbol")
public void testEnumSymbolInUnionTypeSymbols(int line, int col, String name, boolean expIsEnum,
String typeName, List<String> expEnumMembers) {
TypeDefinitionSymbol symbol = (TypeDefinitionSymbol) assertBasicsAndGetSymbol(model, srcFile, line, col, name,
SymbolKind.TYPE_DEFINITION);
assertEquals(symbol.typeDescriptor().typeKind(), TypeDescKind.UNION);
UnionTypeSymbol typeSymbol = (UnionTypeSymbol) symbol.typeDescriptor();
assertEquals(typeSymbol.isEnum(), expIsEnum);
assertEquals(typeSymbol.getEnumSymbol().isPresent(), expIsEnum);
typeSymbol.getEnumSymbol().ifPresent(enumSymbol -> {
assertTrue(enumSymbol.getName().isPresent());
assertEquals(enumSymbol.getName().get(), typeName);
assertEquals(enumSymbol.typeDescriptor().typeKind(), TypeDescKind.UNION);
SemanticAPITestUtils.assertList(enumSymbol.members(), expEnumMembers);
});
}

@DataProvider(name = "DataForEnumInUnionTypeSymbol")
public Object[][] getEnumDataInUnionType() {
return new Object[][]{
{47, 5, "FooState", true, "State", List.of("OPEN", "CLOSED")},
{52, 5, "BarState", false, null, List.of()},
{59, 5, "BazConnectionState", true, "ConnectionState", List.of("OK", "ERROR")},
};
}

public static <T> void assertList(List<T> actualValues, List<T> expectedValues) {
assertEquals(actualValues.size(), expectedValues.size());

Expand Down
19 changes: 19 additions & 0 deletions ...allerina-compiler-api-test/src/test/resources/test-src/symbols/union_type_symbol_test.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,22 @@ type T9 int[]|T8;

type Keyword KEY | boolean | "string" | 100 | "200" | true;
const KEY = "int";

enum State {
OPEN,
CLOSED
}

type FooState State;

const ENABLED = "enabled";
const DISABLED = "disabled";

type BarState ENABLED | DISABLED;

enum ConnectionState {
OK = "1",
ERROR = "0"
}

type BazConnectionState ConnectionState;