Skip to content

feat: Db2 binary host variable support #2337

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

Merged
merged 9 commits into from
Jun 24, 2024
Merged

feat: Db2 binary host variable support #2337

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
39ac9ec
Add support for Binary Host Variables
mm-broadcom Jun 12, 2024
9a442d8
Added bounds check per IBM documentation
mm-broadcom Jun 12, 2024
acbb561
Removed duplicate error warnings
mm-broadcom Jun 12, 2024
066bba1
Merge remote-tracking branch 'upstream/development' into DB2-Binary-H…
mm-broadcom Jun 12, 2024
e6c4be5
Add unit tests, better range checking
mm-broadcom Jun 13, 2024
e90f196
Remove unused message
mm-broadcom Jun 13, 2024
536eff3
Underlines proper offending token
mm-broadcom Jun 13, 2024
d2303c8
Update server/engine/src/main/java/org/eclipse/lsp/cobol/implicitDial…
mm-broadcom Jun 18, 2024
43275ab
Add size requirement for BINARY VARYING
mm-broadcom Jun 19, 2024
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
15 changes: 14 additions & 1 deletion server/engine/src/main/antlr4/org/eclipse/lsp/db2/parser/Db2SqlParser.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,20 @@ execRule: EXEC SQL sqlCode END_EXEC
| {notifyError("cobolParser.missingEndExec");} EXEC SQL sqlCode DOT_FS? EOF
| {notifyError("cobolParser.missingEndExec");} EXEC SQL;

nonExecRule: result_set_locator_host_variable;
nonExecRule: host_variable_rule;

host_variable_rule: (result_set_locator_host_variable | binary_host_variable);

result_set_locator_host_variable: dbs_level_01 entry_name (USAGE IS?)? SQL TYPE IS RESULT_SET_LOCATOR VARYING;

binary_host_variable: dbs_level_01 entry_name host_variable_usage binary_host_variable_type;
binary_host_variable_type: BINARY LPARENCHAR binary_host_variable_binary_size RPARENCHAR | VARBINARY LPARENCHAR binary_host_variable_varbinary_size RPARENCHAR | BINARY VARYING;
Copy link
Contributor

Choose a reason for hiding this comment

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

( BINARY | VARBINARY | BINARY VARYING) LPARENCHAR binary_host_variable_varbinary_size RPARENCHAR

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like 01 VBIN-VAR USAGE IS SQL TYPE IS BINARY VARYING(10). is a valid syntax
Also, this introduces

      7   1  VBIN-VAR. . . . . . . . . . . . . . . . . . . BLL=00001   000000000   DS 0CL12          Group
      0     2  VBIN-VAR-LEN. . . . . . . . . . . . . . . . BLL=00001   000000000   DS 2C             Binary
      0     2  VBIN-VAR-TEXT . . . . . . . . . . . . . . . BLL=00001   000000002   DS 10C            Display

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we know the max length when using BINARY VARYING? I've kept the sizes separate in order to allow for bounds/range checking as BINARY and VARBINARY have different max sizes. (255 and 32704 respectively)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well theoretically, these are semantics check and not a syntax issue. But I don't mind it either ways. As a general rule I like to consolidate sub rules to avoid look ahead burden. This is explained at improving-the-performance-of-an-antlr-parse ( check section - Consolidating sub rules)

Since this is a small rule, it doesn't matter a lot here.

For the semantics check, we can always do these in Visitor as the binary_host_variable parser rule context would have all the appropriate value within it.


binary_host_variable_binary_size: T=dbs_integerliteral_expanded {validateIntegerRange($T.start, $T.text, 1, 255);};
binary_host_variable_varbinary_size: T=dbs_integerliteral_expanded {validateIntegerRange($T.start, $T.text, 1, 32704);};

host_variable_usage: (USAGE IS?)? SQL TYPE IS;

entry_name : (FILLER |dbs_host_names);
sqlCode
: ~END_EXEC*?
Expand Down Expand Up @@ -1783,6 +1794,8 @@ dbs_sql_identifier: NONNUMERICLITERAL | IDENTIFIER | FILENAME | FILENAME (DOT_FS
dbs_comma_separator: (COMMASEPARATORDB2 | COMMACHAR);
dbs_semicolon_end: SEMICOLON_FS | SEMICOLONSEPARATORSQL;

dbs_integerliteral_expanded: MINUSCHAR? (INTEGERLITERAL|SINGLEDIGITLITERAL|SINGLEDIGIT_1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if -ve length should be allowed at grammar level. This would always be +ve integer as per doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct, but if the user types a minus sign by accident we can now catch that. See the screenshot at the bottom of the initial comment/post in the PR as an example.

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense 👍


dbs_integer0: T=dbs_integer {validateValue($T.text, "0");};
dbs_integer1: T=dbs_integer {validateValue($T.text, "1");};
dbs_integer2: T=dbs_integer {validateValue($T.text, "2");};
Expand Down
27 changes: 27 additions & 0 deletions server/engine/src/main/java/org/eclipse/lsp/cobol/implicitDialects/sql/Db2SqlVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,33 @@ public List<Node> visitResult_set_locator_host_variable(
return ImmutableList.of(variableDefinitionNode);
}

@Override
public List<Node> visitBinary_host_variable(Db2SqlParser.Binary_host_variableContext ctx) {
Copy link
Contributor

@ap891843 ap891843 Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Doc suggest this VARBINARY would generate source member as mentioned below
image

This would mean insert new variables.
We can refer Db2ImplicitVariablesGenerator for a similar use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we would need to add test, for usage of generated variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The IBM DB2 preprocessor converts the DB2 variable types to those supported by COBOL. These generated variables should already be taken care of.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is that below program with db2 pre/co-processor would compile fine. But cobol-ls would start to throw error variable not defined.

        Identification Division.
        Program-Id. 'TEST1'.
        Data Division.
         Working-Storage Section.
         LINKAGE SECTION.
         01 VBIN-VAR USAGE IS SQL TYPE IS BINARY VARYING(10).
        PROCEDURE division.
           display  VBIN-VAR-LEN.
           display  VBIN-VAR-TEXT.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take it from here. Let's merge this PR if everything else is fine.

addReplacementContext(ctx);
Locality statementLocality = getLocality(this.context.getExtendedDocument().mapLocation(constructRange(ctx)));

Db2WorkingAndLinkageSectionNode semanticsNode = new Db2WorkingAndLinkageSectionNode(statementLocality);

VariableDefinitionNode variableDefinitionNode =
VariableDefinitionNode.builder()
.level(Integer.parseInt(ctx.dbs_level_01().getText()))
.levelLocality(
getLocality(
this.context.getExtendedDocument().mapLocation(constructRange(ctx.entry_name()))))
.statementLocality(statementLocality)
.variableNameAndLocality(
new VariableNameAndLocality(
VisitorHelper.getName(ctx.entry_name()),
getLocality(
this.context
.getExtendedDocument()
.mapLocation(constructRange(ctx.entry_name())))))
.usageClauses(ImmutableList.of(UsageFormat.DISPLAY))
.build();
variableDefinitionNode.addChild(semanticsNode);
return ImmutableList.of(variableDefinitionNode);
}

private Locality getLocality(Location location) {
Locality.LocalityBuilder builder =
Locality.builder().uri(location.getUri()).range(location.getRange());
Expand Down
51 changes: 51 additions & 0 deletions ...engine/src/main/java/org/eclipse/lsp/cobol/implicitDialects/sql/MessageServiceParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ public void notifyError(String messageId, String... parameters) {
notifyListeners(message);
}

/**
* Extend the functionality of notifyError to include the offending token along with the message.
* @param offendingToken Token where the error occurs
* @param messageId Unique ID for each message in externalized message file.
* @param parameters Arguments referenced by the format specifiers in the format string in
* externalized message file.
*/
public void notifyError(Token offendingToken, String messageId, String... parameters) {
String message = getMessageForParser(messageId, parameters);
notifyErrorListeners(offendingToken, message, null);
}

@Override
public void notifyErrorListeners(Token offendingToken, String msg, RecognitionException e) {
_syntaxErrors++;
Expand Down Expand Up @@ -96,6 +108,35 @@ protected void validateValue(String actual, String expected) {
}
}

/**
* Validate integer value against range and throw an error if it is incorrect
*
* @param input integer to check
* @param minValue allowed integer value
* @param maxValue allowed integer value
*/
protected void validateIntegerRange(String input, Integer minValue, Integer maxValue) {
Integer intInputValue = tryParseInt(input);
if (intInputValue != null && !(intInputValue >= minValue && intInputValue <= maxValue)) {
notifyError("parsers.intRangeValue", minValue.toString(), maxValue.toString());
}
}

/**
* Validate integer value against range and throw an error if it is incorrect
*
* @param start start token
* @param input integer to check
* @param minValue allowed integer value
* @param maxValue allowed integer value
*/
protected void validateIntegerRange(Token start, String input, Integer minValue, Integer maxValue) {
Integer intInputValue = tryParseInt(input);
if (intInputValue != null && !(intInputValue >= minValue && intInputValue <= maxValue)) {
notifyError(start, "parsers.intRangeValue", minValue.toString(), maxValue.toString());
}
}

/**
* Validate that the subschema name is 16 or 18
*
Expand Down Expand Up @@ -221,4 +262,14 @@ private String getMessageForParser(String messageKey, String... parameters) {
.getMessageService()
.getMessage(messageKey, (Object[]) parameters);
}

private Integer tryParseInt(String input) {
Integer parsedValue;
try {
parsedValue = Integer.parseInt(input);
} catch (Exception ex) {
parsedValue = null;
}
return parsedValue;
}
}
97 changes: 97 additions & 0 deletions server/engine/src/test/java/org/eclipse/lsp/cobol/usecases/sql/TestSqlHostVariable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,38 @@ public class TestSqlHostVariable {
+ " PROCEDURE DIVISION.\n"
+ " {_01 VAR-NAME USAGE IS SQL TYPE IS RESULT-SET-LOCATOR VARYING|1_}.";

public static final String BINARY_TEXT1 =
" Identification Division.\n"
+ " Program-Id. 'TEST1'.\n"
+ " Data Division.\n"
+ " Working-Storage Section.\n"
+ " 01 {$*VAR-INVALID-BIN} USAGE IS SQL TYPE IS BINARY({256|1}).\n"
+ " PROCEDURE DIVISION.\n";

public static final String BINARY_TEXT2 =
" Identification Division.\n"
+ " Program-Id. 'TEST1'.\n"
+ " Data Division.\n"
+ " Working-Storage Section.\n"
+ " 01 {$*VAR-INVALID-BIN} USAGE IS SQL TYPE IS BINARY({-|1}1).\n"
+ " PROCEDURE DIVISION.\n";

public static final String BINARY_TEXT3 =
" Identification Division.\n"
+ " Program-Id. 'TEST1'.\n"
+ " Data Division.\n"
+ " Working-Storage Section.\n"
+ " 01 {$*VAR-INVALID-BIN} USAGE IS SQL TYPE IS VARBINARY({32705|1}).\n"
+ " PROCEDURE DIVISION.\n";

public static final String BINARY_TEXT4 =
" Identification Division.\n"
+ " Program-Id. 'TEST1'.\n"
+ " Data Division.\n"
+ " Working-Storage Section.\n"
+ " 01 {$*VAR-INVALID-BIN} USAGE IS SQL TYPE IS VARBINARY({-|1}1234).\n"
+ " PROCEDURE DIVISION.\n";

@Test
void testSupportForResultSetLocator() {
UseCaseEngine.runTest(TEXT, ImmutableList.of(), ImmutableMap.of());
Expand Down Expand Up @@ -80,4 +112,69 @@ void testHostVariable_whenPlacedAtWrongSection() {
DiagnosticSeverity.Error,
ErrorSource.DIALECT.getText())));
}

@Test
void testBinaryHostVariable_whenLargeBinaryValue() {
UseCaseEngine.runTest(
BINARY_TEXT1,
ImmutableList.of(),
ImmutableMap.of(
"1",
new Diagnostic(
new Range(),
"Allowed range is 1 to 255",
DiagnosticSeverity.Error,
ErrorSource.PARSING.getText())));
}

@Test
void testBinaryHostVariable_whenNegativeBinaryValue() {
UseCaseEngine.runTest(
BINARY_TEXT2,
ImmutableList.of(),
ImmutableMap.of(
"1",
new Diagnostic(
new Range(),
"Allowed range is 1 to 255",
DiagnosticSeverity.Error,
ErrorSource.PARSING.getText()
)
)
);
}

@Test
void testBinaryHostVariable_whenLargeVarbinaryValue() {
UseCaseEngine.runTest(
BINARY_TEXT3,
ImmutableList.of(),
ImmutableMap.of(
"1",
new Diagnostic(
new Range(),
"Allowed range is 1 to 32704",
DiagnosticSeverity.Error,
ErrorSource.PARSING.getText()
)
)
);
}

@Test
void testBinaryHostVariable_whenNegativeVarbinaryValue() {
UseCaseEngine.runTest(
BINARY_TEXT4,
ImmutableList.of(),
ImmutableMap.of(
"1",
new Diagnostic(
new Range(),
"Allowed range is 1 to 32704",
DiagnosticSeverity.Error,
ErrorSource.PARSING.getText()
)
)
);
}
}
Loading