-
Notifications
You must be signed in to change notification settings - Fork 802
Add semantic analysis and negative tests for natural expressions #43993
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
Add semantic analysis and negative tests for natural expressions #43993
Conversation
…atform/ballerina-lang into natural-programming
…atform/ballerina-lang into natural-programming
…atform/ballerina-lang into natural-programming
c8fcb0b to
7726ada
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## natural-programming #43993 +/- ##
======================================================
Coverage ? 74.99%
Complexity ? 57662
======================================================
Files ? 3546
Lines ? 223032
Branches ? 28868
======================================================
Hits ? 167264
Misses ? 46493
Partials ? 9275 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| String moduleName, DependencyResolutionType dependencyResolutionType) { | ||
| if (!currentModuleDesc.name().toString().equals(moduleName)) { | ||
| ModuleLoadRequest moduleLoadRequest = new ModuleLoadRequest( | ||
| PackageOrg.from(orgName), moduleName, scope, dependencyResolutionType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In here Don't we need to consider about the language version
As an example np module can have different versions for U12 and U13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's resolved by the project API? Which is why we shouldn't specify a version here?
| */ | ||
| public class NaturalProgrammingImportAnalyzer extends NodeVisitor { | ||
|
|
||
| private boolean shouldImportNaturalProgrammingModule; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private boolean shouldImportNaturalProgrammingModule; | |
| private boolean shouldImportNaturalProgrammingModule = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in #44013.
| EXPECTED_TYPE_FOR_NATURAL_EXPR_MUST_CONTAIN_ERROR("BCE4070", "expected.type.for.natural.expr.must.contain.error"), | ||
| EXPECTED_TYPE_FOR_NATURAL_EXPR_MUST_CONTAIN_A_UNION_OF_NON_ERROR_AND_ERROR( | ||
| "BCE4071", "expected.type.for.natural.expr.must.contain.a.union.of.non.error.and.error"), | ||
| EXPECTED_TYPE_FOR_NATURAL_EXPR_MUST_BE_A_SUBTYPE_OF_ANYDATA_OR_ERROR( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont we need to avoid the regexp in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why though?
|
|
||
| @Override | ||
| public String toString() { | ||
| return "BLangNaturalExpression: strings " + strings + ", insertions " + insertions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont we need to construct the original expression in here, Same thing done by the SQL queries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've kept this consistent with other templates in the language.
| return externalFunctionBodyNode.annotations().stream() | ||
| .anyMatch(annotation -> | ||
| annotation.annotReference() instanceof SimpleNameReferenceNode annotReference && | ||
| CODE_ANNOTATION.equals(annotReference.name().text())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since code is a common name, shall we check the prompt field also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll have the module prefix then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we don't give a redeclared symbol error for code in the current module. Will fix this for m2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is when there is a code annotation in the current module.
|
Adding more tests, WIP. |
|
Merging to trigger a timestamped build - will send a separate PR with tests/addressing other suggestions if any. |
6c68428
into
ballerina-platform:natural-programming
| /* | ||
| * Copyright (c) 2025, WSO2 LLC. (http://www.wso2.org). | ||
| * | ||
| * WSO2 Inc. licenses this file to you under the Apache License, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be LLC
| import java.util.List; | ||
|
|
||
| /** | ||
| * Represents a natural expression. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Represents a natural expression in AST
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, self-explanatory, given where it is and the class name.
...g/src/main/java/org/wso2/ballerinalang/compiler/tree/expressions/BLangNaturalExpression.java
Show resolved
Hide resolved
...rina-lang/src/main/java/io/ballerina/projects/internal/NaturalProgrammingImportAnalyzer.java
Show resolved
Hide resolved
| boolean constNaturalExpr = naturalExpression.constExpr; | ||
|
|
||
| if (!constNaturalExpr && !types.isSubtype(symTable.errorType, expTypeSemType)) { | ||
| dlog.error(naturalExpression.pos, DiagnosticErrorCode.EXPECTED_TYPE_FOR_NATURAL_EXPR_MUST_CONTAIN_ERROR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cant we use the regular incompatible types error here. The main issue is in the error message we say the expected type is incorrect but the position we use for the error msg is the expression pos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I introduced a specific error message. This is a dependently-typed expression, so saying expected anydata found T for the expected type doesn't really work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is, if we consider an assignment, we are saying that the LHS type is incorrect while giving the error on the RHS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a commn concern with dependently-typed expressions. I think the current error explains the issue better to the user compared to giving an error to the expected type. The error needs to work with all kinds of places in which a natural expression is used.
Purpose
$title.
Positive tests will go in the library module.
Fixes #43945