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

Implement semantic api related visitors for BLangListConstructorSpreadOpExpr #36076

Merged
merged 4 commits into from
May 10, 2022
Merged

Implement semantic api related visitors for BLangListConstructorSpreadOpExpr #36076

merged 4 commits into from
May 10, 2022

Conversation

lochana-chathura
Copy link
Member

Purpose

$subject.

Fixes #36073

Approach

N/A

Samples

N/A

Remarks

N/A

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

@lochana-chathura lochana-chathura changed the base branch from master to 2201.1.0-stage May 10, 2022 06:20
@lochana-chathura
Copy link
Member Author

@dulajdilshan @sanjana Do we have semantic api tests for each AST node? Do we need to add test cases with these changes?

@sanjana
Copy link
Contributor

sanjana commented May 10, 2022

@dulajdilshan @sanjana Do we have semantic api tests for each AST node? Do we need to add test cases with these changes?

@lochana-chathura,
Need to add a test case for the references() in Semantic API for the crashing example mentioned in the related issue.

@codecov
Copy link

codecov bot commented May 10, 2022 •
edited
Loading

Codecov Report

Merging #36076 (2d61903) into 2201.1.0-stage (166d837) will decrease coverage by 3.19%.
The diff coverage is 55.55%.

@@                 Coverage Diff                  @@
##             2201.1.0-stage   #36076      +/-   ##
====================================================
- Coverage             73.74%   70.54%   -3.20%     
+ Complexity            47660    45824    -1836     
====================================================
  Files                  3185     3185              
  Lines                186040   186049       +9     
  Branches              24280    24280              
====================================================
- Hits                 137190   131253    -5937     
- Misses                40894    47471    +6577     
+ Partials               7956     7325     -631
Impacted Files Coverage Δ
...ava/io/ballerina/compiler/api/impl/NodeFinder.java 66.09% <0.00%> (-0.23%) ⬇️
...2/ballerinalang/compiler/desugar/QueryDesugar.java 81.44% <0.00%> (-0.29%) ⬇️
...va/io/ballerina/compiler/api/impl/BaseVisitor.java 22.27% <100.00%> (+0.37%) ⬆️
...o/ballerina/compiler/api/impl/ReferenceFinder.java 80.91% <100.00%> (+0.06%) ⬆️
...a/io/ballerina/compiler/api/impl/SymbolFinder.java 75.03% <100.00%> (+0.07%) ⬆️
...rg/ballerinalang/debugadapter/DebugSourceType.java 0.00% <0.00%> (-100.00%) ⬇️
...g/ballerinalang/debugadapter/DebugInstruction.java 0.00% <0.00%> (-100.00%) ⬇️
.../ballerinalang/debugadapter/DebugProjectCache.java 0.00% <0.00%> (-100.00%) ⬇️
.../ballerinalang/debugadapter/EvaluationContext.java 0.00% <0.00%> (-100.00%) ⬇️
...ballerinalang/debugadapter/evaluation/BImport.java 0.00% <0.00%> (-100.00%) ⬇️
... and 235 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 139c1a7...2d61903. Read the comment docs.

@lochana-chathura lochana-chathura changed the title Implement semantic api related visitors for BLangListConstructorSprea… Implement semantic api related visitors for BLangListConstructorSpreadOpExpr May 10, 2022
@lochana-chathura lochana-chathura added this to the Ballerina 2201.1.0 milestone May 10, 2022
@pcnfernando
Copy link
Member

Try to handle the uncovered test scenarios for SemanticApi and QueryDesugar

sanjana
sanjana suggested changes May 10, 2022
View reviewed changes
...llerina-compiler-api-test/src/test/resources/test-src/find-all-ref/find_var_ref_in_exprs.bal Outdated Show resolved Hide resolved
...llerina-compiler-api-test/src/test/resources/test-src/find-all-ref/find_var_ref_in_exprs.bal Outdated Show resolved Hide resolved
...llerina-compiler-api-test/src/test/resources/test-src/find-all-ref/find_var_ref_in_exprs.bal Outdated Show resolved Hide resolved
tests/ballerina-compiler-api-test/src/test/resources/test-src/symbol_at_cursor_basic_test.bal Outdated Show resolved Hide resolved
tests/ballerina-compiler-api-test/src/test/resources/test-src/symbol_at_cursor_basic_test.bal Outdated Show resolved Hide resolved
tests/ballerina-compiler-api-test/src/test/resources/test-src/symbol_at_cursor_basic_test.bal Outdated Show resolved Hide resolved
malinthar
malinthar approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@malinthar malinthar left a comment

Choose a reason for hiding this comment

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

LGTM!

...ore/src/test/resources/completion/expression_context/source/list_constructor_ctx_source5.bal
@@ -0,0 +1,4 @@
function testListConstructorSpreadOp() {
int[] x = [1, 2];
int[] y = [...];
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to fix the sorting of the completions at the cursor position after .... Further, we may have to add a spread op completion item as well (Just like spread field completion item for mapping constructors). Will create a separate issue for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack. thanks!.

Copy link
Contributor

Choose a reason for hiding this comment

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

#35507 tracks the changes to be made

@lochana-chathura lochana-chathura requested a review from sanjana May 10, 2022 09:51
sanjana
sanjana approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@sanjana sanjana left a comment

Choose a reason for hiding this comment

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

LGTM

@pcnfernando pcnfernando merged commit 66dc53c into ballerina-platform:2201.1.0-stage May 10, 2022
@lochana-chathura lochana-chathura mentioned this pull request May 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malinthar malinthar malinthar approved these changes

@lasinicl lasinicl lasinicl approved these changes

@pcnfernando pcnfernando pcnfernando approved these changes

@IMS94 IMS94 IMS94 approved these changes

@sanjana sanjana sanjana approved these changes

@sameerajayasoma sameerajayasoma Awaiting requested review from sameerajayasoma

@hasithaa hasithaa Awaiting requested review from hasithaa

@dulajdilshan dulajdilshan Awaiting requested review from dulajdilshan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Ballerina 2201.1.0
Development

Successfully merging this pull request may close these issues.

ReferenceFinder crashing with the list constructor spread operator
6 participants
@lochana-chathura @sanjana @pcnfernando @IMS94 @lasinicl @malinthar