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

Validate expr with check keyword in call stmt #29282

Merged
merged 9 commits into from
Mar 26, 2021

Conversation

chiranSachintha
Copy link
Member

Purpose

$title.
Fix the issue mentioned in #28769 (comment) also.

Fixes #28769

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

@chiranSachintha chiranSachintha added the Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. label Mar 17, 2021
@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #29282 (c4375ab) into master (89a036a) will increase coverage by 1.31%.
The diff coverage is 95.83%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #29282      +/-   ##
============================================
+ Coverage     66.77%   68.09%   +1.31%     
- Complexity    34220    35275    +1055     
============================================
  Files          2800     2816      +16     
  Lines        150969   152586    +1617     
  Branches      18251    18534     +283     
============================================
+ Hits         100813   103902    +3089     
+ Misses        44161    42528    -1633     
- Partials       5995     6156     +161     
Impacted Files Coverage Δ Complexity Δ
...erinalang/util/diagnostic/DiagnosticErrorCode.java 99.81% <ø> (-0.01%) 5.00 <0.00> (ø)
...rina/compiler/internal/parser/BallerinaParser.java 85.68% <90.00%> (+0.05%) 2127.00 <5.00> (+14.00)
...alang/compiler/semantics/analyzer/TypeChecker.java 84.67% <100.00%> (-0.03%) 1425.00 <0.00> (+7.00) ⬇️
...iler/internal/diagnostics/DiagnosticErrorCode.java 99.63% <100.00%> (+<0.01%) 5.00 <0.00> (ø)
...allerinalang/central/client/BuildLogFormatter.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...ang/langserver/completions/TypeCompletionItem.java 0.00% <0.00%> (-75.00%) 0.00% <0.00%> (-1.00%)
...xplicitAnonymousFunctionExpressionNodeContext.java 10.00% <0.00%> (-35.00%) 1.00% <0.00%> (-3.00%)
...ns/providers/context/OrderByClauseNodeContext.java 58.53% <0.00%> (-32.77%) 9.00% <0.00%> (+1.00%) ⬇️
...oviders/context/FunctionDefinitionNodeContext.java 21.73% <0.00%> (-26.09%) 2.00% <0.00%> (-3.00%)
.../compiler/semantics/model/types/BReadonlyType.java 43.75% <0.00%> (-25.00%) 3.00% <0.00%> (-1.00%)
... and 291 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 89a036a...c4375ab. Read the comment docs.

@hasithaa hasithaa added this to the Ballerina Swan Lake - Alpha4 milestone Mar 26, 2021
Copy link
Contributor

@rdulmina rdulmina left a comment

Choose a reason for hiding this comment

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

Parser changes LGTM

"operator: all expression types are equivalent to error type", 16, 25);
BAssertUtil.validateError(compile, 2, "invalid usage of the 'check' expression " +
"operator: all expression types are equivalent to error type", 30, 25);
BAssertUtil.validateError(compile, 1, "incompatible types: expected 'string', found 'never'", 16, 19);
Copy link
Contributor

Choose a reason for hiding this comment

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

Example is here is

function testCheckedExprSemanticErrors2() returns error? {
    string line = checkpanic readLineError();
}

The diagnostic is correct, But I think we can improve 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.

@hasithaa Shall I fix this in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. Please create an issue, we can fix this later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created.(#29696)

hasithaa
hasithaa previously approved these changes Mar 26, 2021
Copy link
Contributor

@hasithaa hasithaa left a comment

Choose a reason for hiding this comment

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

Looks good to me

@@ -431,7 +431,6 @@

// Checked expression related errors
CHECKED_EXPR_INVALID_USAGE_NO_ERROR_TYPE_IN_RHS("BCE3030", "checked.expr.invalid.usage.no.error.type.rhs"),
CHECKED_EXPR_INVALID_USAGE_ALL_ERROR_TYPES_IN_RHS("BCE3031", "checked.expr.invalid.usage.only.error.types.rhs"),
Copy link
Member

Choose a reason for hiding this comment

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

Shall we fix the subsequent diagnostic IDs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we update this later, This might conflict with other PRs.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflicting error messages using check inside a type guard
5 participants