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

Jump to bottom

[WIP] Support constant spread-list-member within the keys of a table #42388

Closed
wants to merge 1 commit into from
Closed

[WIP] Support constant spread-list-member within the keys of a table #42388

wants to merge 1 commit into from

Conversation

nipunayf
Copy link
Contributor

@nipunayf nipunayf commented Mar 22, 2024 •
edited
Loading

Purpose

As mentioned in the spec, structural constructors are allowed as long as their subexpressions are also constant. However, as mentioned in #41979, an invalid diagnostic is produced when a spread-list-member is within such constructors.

Related to: #41992

Fixes #41979

Check List

  • Read the Contributing Guide
  • Updated Change Log
  • Checked Tooling Support (#)
  • Added necessary tests
    • Unit Tests
    • Spec Conformance Tests
    • Integration Tests
    • Ballerina By Example Tests
  • Increased Test Coverage
  • Added necessary documentation
    • API documentation
    • Module documentation in Module.md files
    • Ballerina By Examples

@nipunayf nipunayf added the Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. label Mar 22, 2024
@nipunayf nipunayf changed the title Support spread-list-member within the keys of a table Support constant spread-list-member within the keys of a table Mar 22, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 22, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.50%. Comparing base (4b24170) to head (87a2eda).
Report is 1780 commits behind head on master.

Files with missing lines Patch % Lines
.../compiler/semantics/analyzer/DataflowAnalyzer.java 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##             master   #42388   +/-   ##
=========================================
  Coverage     76.50%   76.50%           
- Complexity    53178    53183    +5     
=========================================
  Files          2892     2892           
  Lines        201004   201004           
  Branches      26188    26189    +1     
=========================================
+ Hits         153771   153782   +11     
+ Misses        38772    38762   -10     
+ Partials       8461     8460    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KavinduZoysa KavinduZoysa added this to the 2201.9.0 milestone Mar 25, 2024
MaryamZi
MaryamZi reviewed Mar 25, 2024
View reviewed changes
...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/DataflowAnalyzer.java Show resolved Hide resolved
...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/DataflowAnalyzer.java
} else {
dlog.error(((BLangExpression) node).pos, DiagnosticErrorCode.EXPRESSION_IS_NOT_A_CONSTANT_EXPRESSION);
dlog.error(node.getPosition(), DiagnosticErrorCode.EXPRESSION_IS_NOT_A_CONSTANT_EXPRESSION);
Copy link
Member

Choose a reason for hiding this comment

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

Since there's a codecov warning, can we check if we have tests for this?

We can add a test with a non-const spread expression too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, this line was added as a fail-safe mechanism, and all non-constant expressions are handled through:

ballerina-lang/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java

Line 1516 in 5c364c4

private boolean isConstExpression(BLangExpression expression) {

We also have negative tests for each non-constant spread expression.

ballerina-lang/tests/jballerina-unit-test/src/test/resources/test-src/types/table/table-negative.bal

Line 588 in e3f55f4

{k: [12, ...intArr], value: 17}

ballerina-lang/tests/jballerina-unit-test/src/test/resources/test-src/types/table/table-negative.bal

Line 608 in e3f55f4

{...mapVal}

...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/DataflowAnalyzer.java
Comment on lines +1444 to +1447
} else if (node.getKind() == NodeKind.LIST_CONSTRUCTOR_SPREAD_OP) {
result = 31 * result + hash(((BLangListConstructorExpr.BLangListConstructorSpreadOpExpr) node).expr);
} else if (node.getKind() == NodeKind.RECORD_LITERAL_SPREAD_OP) {
result = 31 * result + hash(((BLangRecordLiteral.BLangRecordSpreadOperatorField) node).expr);
Copy link
Member

Choose a reason for hiding this comment

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

Case 1 - These wouldn't be correct/sufficient with something like

type Row record {|
    readonly int[]|int[][] k;
    int value;
|};

const int[] INT_ARR = [1, 2];

public function main() {
    table<Row> key (k) _ = table [
        {k: [...INT_ARR], value: 0},
        {k: [INT_ARR], value: 0}
    ];
}

right?

Crashes anyway

[2024-03-25 11:10:41,566] SEVERE {b7a.log.crash} - Cannot read field "tag" because the return value of "org.wso2.ballerinalang.compiler.semantics.analyzer.Types.getImpliedType(org.wso2.ballerinalang.compiler.semantics.model.types.BType)" is null 
java.lang.NullPointerException: Cannot read field "tag" because the return value of "org.wso2.ballerinalang.compiler.semantics.analyzer.Types.getImpliedType(org.wso2.ballerinalang.compiler.semantics.model.types.BType)" is null
        at org.wso2.ballerinalang.compiler.semantics.analyzer.Types.isValueType(Types.java:443)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.Types.isSameType(Types.java:422)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.Types.isSameType(Types.java:385)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.Types.isAssignable(Types.java:858)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.Types.isAssignable(Types.java:846)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.getTypeEquality(DataflowAnalyzer.java:1530)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.equality(DataflowAnalyzer.java:1286)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.checkForKeyEquality(DataflowAnalyzer.java:1139)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.checkForDuplicateKeys(DataflowAnalyzer.java:1122)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.visit(DataflowAnalyzer.java:1111)
        at org.wso2.ballerinalang.compiler.tree.expressions.BLangTableConstructorExpr.accept(BLangTableConstructorExpr.java:70)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.analyzeNode(DataflowAnalyzer.java:2532)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.visit(DataflowAnalyzer.java:690)
        at org.wso2.ballerinalang.compiler.tree.BLangSimpleVariable.accept(BLangSimpleVariable.java:54)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.analyzeNode(DataflowAnalyzer.java:2532)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.visit(DataflowAnalyzer.java:653)
        at org.wso2.ballerinalang.compiler.tree.statements.BLangSimpleVariableDef.accept(BLangSimpleVariableDef.java:50)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.analyzeNode(DataflowAnalyzer.java:2532)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.visit(DataflowAnalyzer.java:472)
        at org.wso2.ballerinalang.compiler.tree.BLangBlockFunctionBody.accept(BLangBlockFunctionBody.java:60)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.analyzeNode(DataflowAnalyzer.java:2532)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.analyzeBranch(DataflowAnalyzer.java:2500)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.visit(DataflowAnalyzer.java:429)
        at org.wso2.ballerinalang.compiler.tree.BLangFunction.accept(BLangFunction.java:76)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.analyzeNode(DataflowAnalyzer.java:2532)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.visit(DataflowAnalyzer.java:344)
        at org.wso2.ballerinalang.compiler.tree.BLangPackage.accept(BLangPackage.java:167)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.analyzeNode(DataflowAnalyzer.java:2532)
        at org.wso2.ballerinalang.compiler.semantics.analyzer.DataflowAnalyzer.analyze(DataflowAnalyzer.java:314)
        at io.ballerina.projects.internal.CompilerPhaseRunner.dataflowAnalyze(CompilerPhaseRunner.java:196)
        at io.ballerina.projects.internal.CompilerPhaseRunner.performTypeCheckPhases(CompilerPhaseRunner.java:120)
        at io.ballerina.projects.ModuleContext.compileInternal(ModuleContext.java:435)
        at io.ballerina.projects.ModuleCompilationState$1.compile(ModuleCompilationState.java:45)
        at io.ballerina.projects.ModuleContext.compile(ModuleContext.java:383)
        at io.ballerina.projects.PackageCompilation.compileModulesInternal(PackageCompilation.java:208)
        at io.ballerina.projects.PackageCompilation.compileModules(PackageCompilation.java:192)
        at io.ballerina.projects.PackageCompilation.compile(PackageCompilation.java:99)
        at io.ballerina.projects.PackageCompilation.from(PackageCompilation.java:94)
        at io.ballerina.projects.PackageContext.getPackageCompilation(PackageContext.java:243)
        at io.ballerina.projects.Package.getCompilation(Package.java:150)
        at io.ballerina.projects.Package.runCodeGeneratorPlugins(Package.java:323)
        at io.ballerina.cli.task.CompileTask.execute(CompileTask.java:144)
        at io.ballerina.cli.TaskExecutor.executeTasks(TaskExecutor.java:40)
        at io.ballerina.cli.cmd.RunCommand.execute(RunCommand.java:247)
        at java.base/java.util.Optional.ifPresent(Optional.java:178)
        at io.ballerina.cli.launcher.Main.main(Main.java:58)
 
ERROR [.:(1:1,1:1)] Compilation failed due to: Cannot read field "tag" because the return value of "org.wso2.ballerinalang.compiler.semantics.analyzer.Types.getImpliedType(org.wso2.ballerinalang.compiler.semantics.model.types.BType)" is null

Copy link
Member

Choose a reason for hiding this comment

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

Also crashes for

type Row record {|
    readonly int[] k;
    int value;
|};

const int[] INT_ARR = [1, 2];

public function main() {
    table<Row> key (k) _ = table [
        {k: [...INT_ARR], value: 0},
        {k: [...INT_ARR], value: 0}
    ];
}

...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/DataflowAnalyzer.java
Comment on lines +1444 to +1447
} else if (node.getKind() == NodeKind.LIST_CONSTRUCTOR_SPREAD_OP) {
result = 31 * result + hash(((BLangListConstructorExpr.BLangListConstructorSpreadOpExpr) node).expr);
} else if (node.getKind() == NodeKind.RECORD_LITERAL_SPREAD_OP) {
result = 31 * result + hash(((BLangRecordLiteral.BLangRecordSpreadOperatorField) node).expr);
Copy link
Member

Choose a reason for hiding this comment

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

Case 2 - same key isn't detected at compile-time when one is via spread and the other is via values defined directly.

type Row record {|
    readonly int[] k;
    int value;
|};

const int[] INT_ARR = [1, 2];

public function main() {
    table<Row> key (k) _ = table [
        {k: [...INT_ARR], value: 0},
        {k: [1, 2], value: 0}
    ];
}

Fails at runtime instead.

Copy link
Contributor Author

@nipunayf nipunayf Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is not specific to the spread op. The same concern can be reproduced with a simple constant, and the 'duplicate key' error is not generated at compile time.

type Row record {|
    readonly int k;
    int value;
|};

const VAL = 1;

public function main() {
    table<Row> key (k) _ = table [
        {k: 1, value: 0},
        {k: VAL, value: 0}
    ];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appears that this is a known issue: #35584

@nipunayf nipunayf marked this pull request as draft March 25, 2024 12:02
@nipunayf nipunayf removed this from the 2201.9.0 milestone Mar 27, 2024
@github-actions GitHub Actions
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Apr 11, 2024
@nipunayf nipunayf removed the Stale label Apr 12, 2024
@github-actions GitHub Actions
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Apr 27, 2024
@github-actions GitHub Actions
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label May 13, 2024
@nipunayf nipunayf removed the Stale label May 14, 2024
@nipunayf nipunayf changed the title Support constant spread-list-member within the keys of a table [WIP] Support constant spread-list-member within the keys of a table May 14, 2024
@github-actions GitHub Actions
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label May 29, 2024
@nipunayf nipunayf removed the Stale label May 29, 2024
@github-actions GitHub Actions
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Jun 14, 2024
@nipunayf nipunayf removed the Stale label Jun 15, 2024
@nipunayf nipunayf added Lang/Table Type `table` related issues Area/CodeAnalysis Data flow , Code and Closure analysis related issues #Compiler labels Jun 26, 2024
@github-actions GitHub Actions
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Jul 12, 2024
@nipunayf nipunayf removed the Stale label Jul 13, 2024
@github-actions GitHub Actions
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Jul 28, 2024
@nipunayf nipunayf removed the Stale label Jul 29, 2024
@github-actions GitHub Actions
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Aug 14, 2024
@nipunayf nipunayf removed the Stale label Aug 14, 2024
@github-actions GitHub Actions
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Aug 30, 2024
@nipunayf nipunayf removed the Stale label Aug 31, 2024
@github-actions GitHub Actions
Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label Sep 15, 2024
@gimantha gimantha added the IceBox Older issues that are not being actively worked on but may be revisited in the future. label Oct 2, 2024
@gimantha
Copy link
Contributor

gimantha commented Oct 2, 2024

Need to revisit the design again. Till then we will be closing this PR.

@gimantha gimantha closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaryamZi MaryamZi MaryamZi left review comments

@KavinduZoysa KavinduZoysa KavinduZoysa approved these changes

@sameerajayasoma sameerajayasoma Awaiting requested review from sameerajayasoma sameerajayasoma is a code owner

@hasithaa hasithaa Awaiting requested review from hasithaa hasithaa is a code owner

@LakshanWeerasinghe LakshanWeerasinghe Awaiting requested review from LakshanWeerasinghe

@mindula mindula Awaiting requested review from mindula

@gimantha gimantha Awaiting requested review from gimantha gimantha will be requested when the pull request is marked ready for review gimantha is a code owner

@warunalakshitha warunalakshitha Awaiting requested review from warunalakshitha warunalakshitha will be requested when the pull request is marked ready for review warunalakshitha is a code owner

@azinneera azinneera Awaiting requested review from azinneera azinneera will be requested when the pull request is marked ready for review azinneera is a code owner

Assignees
No one assigned
Labels
Area/CodeAnalysis Data flow , Code and Closure analysis related issues #Compiler IceBox Older issues that are not being actively worked on but may be revisited in the future. Lang/Table Type `table` related issues Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: An invalid error is generated when using a constant expression in a spread operator
4 participants
@nipunayf @gimantha @MaryamZi @KavinduZoysa