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

[FEATURE] Label jumping out of code block #105

Open
brentlblair opened this issue Jun 3, 2020 · 34 comments
Open

[FEATURE] Label jumping out of code block #105

brentlblair opened this issue Jun 3, 2020 · 34 comments
Assignees
Labels
enhancement New feature or request

Comments

@brentlblair
Copy link
Contributor

Describe the bug
Lint shows an error on both lines with "GOTO 208".
208 is trying to go out of scope ex
TEST(24, 20): Invalid GOTO or GOSUB, jumping into/out of a block
This code compiles without issue and works in our production jBase environment. In this snippet, we want to jump back to the input if we receive bad input from the user. I took this snippet from an existing program and just copied this portion of code to make sure that it was not falsely complaining about other code in the program.

Code samples and/or reproduceable test cases
FOR P=1 TO NUM.LINES
TOT.PICKED=TEMP2<P,3> ;*Picked = Shipped
IF TOT.PICKED > "0" THEN ;Don't put back parts not yet picked
PN=TEMP2<P,2>
SCANNED.BIN=TEMP2<P,1>
LOC=TEMP2<P,20>
READ PRD FROM F.PRODUCT,PN:"
":LOC ELSE PRD=""
MFG=PRD<4>
PART.NUM=PRD<19>
BIN1=PRD<60,1>
BIN2=PRD<60,2>
PRINT @(0,1):WIPE:"Reverse Pick"
PRINT @(0,2):WIPE:"Loc ":LOC
PRINT @(0,3):WIPE:"Part# ":MFG:PART.NUM
PRINT @(0,4):WIPE:"QTY ":TOT.PICKED
PRINT @(0,5):WIPE:"Bin1 ":BIN1
PRINT @(0,6):WIPE:"Bin2 ":BIN2
208 PRINT BOTTOM.POS:"Bin Location":
INPUT SOURCE.DESTINATION:
SOURCE.DESTINATION.MODE="D"
IF SOURCE.DESTINATION.ERROR#"" THEN
PRINT BOTTOM.POS:SOURCE.DESTINATION.ERROR
RQM(2)
GOTO 208
END
THIS.SCAN.QTY=TOT.PICKED
LOC.QTY.MODE="R"
IF LOC.QTY.ERROR#"" THEN
PRINT BOTTOM.POS:LOC.QTY.ERROR
RQM(2)
GOTO 208
END
END
NEXT P

@kpowick kpowick self-assigned this Jun 3, 2020
@kpowick kpowick added the bug Something isn't working label Jun 3, 2020
@kpowick kpowick added this to the v2.0.7 milestone Jun 3, 2020
@kpowick
Copy link
Member

kpowick commented Jun 3, 2020

Thanks, @brentlblair . This issue is related to issues #99 and #104, which I hope to resolve for the v1.0.7 release.

@dthiot
Copy link
Contributor

dthiot commented Jun 3, 2020 via email

@dthiot
Copy link
Contributor

dthiot commented Jun 3, 2020 via email

@brentlblair
Copy link
Contributor Author

brentlblair commented Jun 3, 2020 via email

@kpowick
Copy link
Member

kpowick commented Jun 3, 2020 via email

@dthiot
Copy link
Contributor

dthiot commented Jun 3, 2020 via email

@kpowick
Copy link
Member

kpowick commented Jun 3, 2020 via email

@ianmcgowan
Copy link
Contributor

Thanks, @brentlblair . This issue is related to issues #99 and #104, which I hope to resolve for the v1.0.7 release.

Kevin, not sure this is related to those issues, there's explicit code to check for jumping into or out of a block (technically any code at a different indent level) in the linter. I sort of agree with the intent of that, but it's quite opinionated of the extension to flag it as an error - it's legal syntax and there's plenty of legacy code around that does worse :_) The intent of this check is good, but it's perhaps a mistake to flag legal code as an error (maybe a warning makes more sense?).

                if (
                  labelMatch.Level != RowLevel[i] &&
                  labelMatch.Level > 1 &&
                  ignoreGotoScope === false
                ) {
                  // jumping into or out of a loop

Here's a more minimal test case that is legal syntax, no label issues, but flags the goto as an error:

FOR X=1 TO 20
  CRT X
  IF X > 10 THEN
LOOPING:
    CRT 'ENTER / TO QUIT: X=':X
    INPUT AAA
    IF AAA#'/' THEN
      GOTO LOOPING
    END
  END
NEXT X

Because the checking is happening based purely on tracking the "level" of each line, you can fool this check by adding a dummy for loop to push the label to the same level as the goto and the error goes away.

FOR X=1 TO 20
  IF X > 10 THEN
    FOR F=1 TO 1
LOOPING:
    NEXT F
    PRINT 'ENTER / TO QUIT: X=':X
    INPUT AAA
    IF AAA#'/' THEN
      GOTO LOOPING
    END
  END
NEXT X

@brentlblair
Copy link
Contributor Author

brentlblair commented Jun 5, 2020 via email

@kpowick
Copy link
Member

kpowick commented Jun 5, 2020 via email

@ianmcgowan
Copy link
Contributor

ianmcgowan commented Jun 5, 2020 via email

@brentlblair
Copy link
Contributor Author

brentlblair commented Jun 5, 2020 via email

@pschellenbach
Copy link

This setting may need to have 3 choices: ignore, warning or error. The MVON# extension is the ancestor of this extension, and in the MVON# version of BASIC, jumping out of a code block is indeed an error. For other MV flavors it could be a warning or just ignored depending on policy.

@ianmcgowan
Copy link
Contributor

I completely agree and was actually thinking of removing it entirely. It might be "bad form", but if the compiler allows it, the extension shouldn't flag it as an error. I think that future versions of the extension could include options to warn about such coding practices, but not at this stage of its development.

Especially since the linting is completely "lexical" and doesn't really consider the structure of the program at all. It would be awesome to actually parse the program into tokens and make the linting more aware of the structure, but that's a non-trivial task. Pete S. has a parser written in dotnet that might make a starting point after converting to typescript.

@ianmcgowan
Copy link
Contributor

This setting may need to have 3 choices: ignore, warning or error. The MVON# extension is the ancestor of this extension, and in the MVON# version of BASIC, jumping out of a code block is indeed an error. For other MV flavors it could be a warning or just ignored depending on policy.

That's good to know! I retract my "opinionated" comment :-) It does make clear how challenging parsing the different flavors will be. I'd vote for vanilla-R83 linting to start!

@dthiot
Copy link
Contributor

dthiot commented Jun 5, 2020 via email

@ianmcgowan
Copy link
Contributor

My vote would be to add extension settings to control this. I use PHPStorm, and it has a whole section of user settings around linter. I don’t want to see this flagged as an error or warning, whereas other users might. I have to imagine there will be other similar situations with the linter where we could all debate what should be considered a warning, error, or nothing.

That's a really good idea. I can imagine a lot more settings like ignoreGotoScope. And it would be nice to have a hierarchy if possible: MVBasic.linting.GotoScope = "warning", MVBasic.linting.MissingLabels = "ignore" etc. Might skip some goto debates that way, and also provide an option for folks with ancient codebases to turn off the nagging.

@kpowick
Copy link
Member

kpowick commented Jun 5, 2020

There seems to be several things to consider here. Since we have the ignoreGoToScope option, perhaps in the interest of getting v2.0.7 out soon, we should revisit this item for v2.0.8. Though maybe for now, the linter can flag this as an warning instead of an error.

@ianmcgowan
Copy link
Contributor

There seems to be several things to consider here. Since we have the ignoreGoToScope option, perhaps in the interest of getting v.1.0.7 out soon, we should revisit this item for v 1.0.8. Though maybe for now, the linter can flag this as an warning instead of an error.

Agreed. Brett is ok with using the option for now, and mass changes to the linter is something that will take a while to work through and test (if it's even considered a good idea). Vote to close?

@kpowick
Copy link
Member

kpowick commented Jun 5, 2020

Vote to close?

I'll just remove the 2.0.7 milestone and change it from a bug to an FEATURE request.

@kpowick kpowick added enhancement New feature or request and removed bug Something isn't working labels Jun 5, 2020
@kpowick kpowick removed this from the v2.0.7 milestone Jun 5, 2020
@kpowick kpowick changed the title [BUG] Label jumping out of code block [ENHANCEMENT] Label jumping out of code block Jun 5, 2020
@kpowick kpowick changed the title [ENHANCEMENT] Label jumping out of code block [FEATURE] [was bug] Label jumping out of code block Jun 5, 2020
@kpowick
Copy link
Member

kpowick commented Jun 10, 2020

For 2.0.7, this scope jumping message from the linter is changed from an Error to a Warning. As noted, this check can be enabled/disabled via the extension settings option ignoreGoToScope.

kpowick pushed a commit that referenced this issue Jun 10, 2020
- Issue #45: Alignment of If/Else
- Issue #96: Allow periods in labels
- Issue #99: Corrected Label error with GOTO/GOSUB.
- Issue #104: Corrected For/Next with leading label
- Issue #105: Changed GOTO scope jumping message from Error to Warning
- RegEx: Label comments to include "REM, *, and !"
- NEXT can be used without FOR variable.
itsxallwater added a commit that referenced this issue Jun 11, 2020
* RegEx fix for labels with leading spaces.

* 98-Issue linting with multiple levels of quoting

* RegEx improvement for quoted strings

* Replace parenthesis and quoted strings with empty ones.
- Removed repetitive Trim()s
- Removed unnecessary RegEx tests
- Simplified line split on semicolon due to above changes.
- Results: Faster execution. Better coverage for split on semicolon

* - Cleanup lines[] array at start so repeated test/replace not needed
- Removed unnecessary trim()s
- Removed boolean equality tests. if(x.test == true) vs if(x.test)
- Fixed GOTO/GOSUB when followed by more than one space GOTO <s><s> 100

* RegEx fix-ups to support recent changes

* Corrected comment

* Linter RegEx cleanup

* FOR/NEXT tracking improvements and updated error messages
- Nested For/Next statements now validated
- NEXT with no variable is now allowed
- One line per error: Removed extraneous, repeated diagnostic messages
- Reworded / Clarified some error messages for consistency

* Minor code clean-up

* Replace char-at-a-time split on ; with split(";")

* Maintain original character spacing for Intellisense error markers
- RegEx improvements and consistency between client and server
- Improved getWord() function

* Corrected indentation for ELSE END with trailing comment (issue #45)

* Corrects Label error with GOTO/GOSUB. Issue #99
- Also now allows dotted numeric labels such as GOTO 100.5

* Revert "Corrects Label error with GOTO/GOSUB. Issue #99"

This reverts commit 68b0166.

* Inspect TM Scopes->Inspect Editor Token and Scopes

* Fix error processing customWordPath when last
line is blank. May relate to #24.

* Changed GOTO/GOSUB scope jumping message from Error to Warning

* Numerous updates related to For/Next and Label tracking
- Issue #45: Alignment of If/Else
- Issue #96: Allow periods in labels
- Issue #99: Corrected Label error with GOTO/GOSUB.
- Issue #104: Corrected For/Next with leading label
- Issue #105: Changed GOTO scope jumping message from Error to Warning
- RegEx: Label comments to include "REM, *, and !"
- NEXT can be used without FOR variable.

* Shipping 2.0.7

Co-authored-by: Kevin Powick <kpowick@gmail.com>
Co-authored-by: Ian McGowan <ian.mcgowan@gmail.com>
Co-authored-by: Peter Schellenbach <petes@zumasys.com>
@kpowick kpowick added this to the v2.0.8 milestone Jun 11, 2020
@brentlblair
Copy link
Contributor Author

brentlblair commented Jun 11, 2020 via email

@dthiot
Copy link
Contributor

dthiot commented Jun 11, 2020 via email

@brentlblair
Copy link
Contributor Author

brentlblair commented Jun 11, 2020 via email

@kpowick
Copy link
Member

kpowick commented Jun 11, 2020

@brentlblair Wow. That is terrible, and I'm sorry.

I tested a bunch of sample programs and coding styles, but none where people had no whitespace between their equal signs -- FOR X=1 TO 10 vs FOR X = 1 TO 10.

Short of getting out a new release, or hotfix, this problem can be resolved by adding the whitespace around your equal signs. However, I understand this may be a coding style change you do not wish to make.

Again, I'm sorry about this. I'll get it fixed-up ASAP and we'll, hopefully, get @itsxallwater to build a hotfix that you can download.

@itsxallwater
Copy link
Member

Yeah we can get a release in to address this quickly. Thanks for jumping on it guys.

@brentlblair
Copy link
Contributor Author

brentlblair commented Jun 11, 2020 via email

@kpowick
Copy link
Member

kpowick commented Jun 11, 2020

@brentlblair

I've opened a new issue #107 for this bug.

Again, sorry. Thanks for your understanding.

@dthiot
Copy link
Contributor

dthiot commented Jun 11, 2020 via email

@kpowick
Copy link
Member

kpowick commented Jun 11, 2020

@dthiot I'm unsure. I've got version 2.0.7 as auto-updated by VS Code and the error happens for me as Brent describes.

I have a screenshot you can review in issue #107. Let's continue this discussion under that issue number.

@itsxallwater
Copy link
Member

Sorry if I'm being dense, this ticket is still open too are we leaving it as-is to address a separate issue?

@kpowick
Copy link
Member

kpowick commented Jun 11, 2020

This was an enhancement request related to Scope Jumping. Unfortunately, we've gone a little OT in the comments, which is why I opened issue #107.

Maybe I should close this one and open a new "clean" enhancement request for Scope Jumping?

@itsxallwater
Copy link
Member

I'm good with it. Thanks for staying on point @kpowick!

itsxallwater added a commit that referenced this issue Jun 11, 2020
* RegEx fix for labels with leading spaces.

* 98-Issue linting with multiple levels of quoting

* RegEx improvement for quoted strings

* Replace parenthesis and quoted strings with empty ones.
- Removed repetitive Trim()s
- Removed unnecessary RegEx tests
- Simplified line split on semicolon due to above changes.
- Results: Faster execution. Better coverage for split on semicolon

* - Cleanup lines[] array at start so repeated test/replace not needed
- Removed unnecessary trim()s
- Removed boolean equality tests. if(x.test == true) vs if(x.test)
- Fixed GOTO/GOSUB when followed by more than one space GOTO <s><s> 100

* RegEx fix-ups to support recent changes

* Corrected comment

* Linter RegEx cleanup

* FOR/NEXT tracking improvements and updated error messages
- Nested For/Next statements now validated
- NEXT with no variable is now allowed
- One line per error: Removed extraneous, repeated diagnostic messages
- Reworded / Clarified some error messages for consistency

* Minor code clean-up

* Replace char-at-a-time split on ; with split(";")

* Maintain original character spacing for Intellisense error markers
- RegEx improvements and consistency between client and server
- Improved getWord() function

* Corrected indentation for ELSE END with trailing comment (issue #45)

* Corrects Label error with GOTO/GOSUB. Issue #99
- Also now allows dotted numeric labels such as GOTO 100.5

* Revert "Corrects Label error with GOTO/GOSUB. Issue #99"

This reverts commit 68b0166.

* Inspect TM Scopes->Inspect Editor Token and Scopes

* Fix error processing customWordPath when last
line is blank. May relate to #24.

* Changed GOTO/GOSUB scope jumping message from Error to Warning

* Numerous updates related to For/Next and Label tracking
- Issue #45: Alignment of If/Else
- Issue #96: Allow periods in labels
- Issue #99: Corrected Label error with GOTO/GOSUB.
- Issue #104: Corrected For/Next with leading label
- Issue #105: Changed GOTO scope jumping message from Error to Warning
- RegEx: Label comments to include "REM, *, and !"
- NEXT can be used without FOR variable.

* Shipping 2.0.7

* Re-introduce accidentally deleted /doc files

* Fixes issue 107: FOR/NEXT error when no whitespace after FOR variable.

* Shipping 2.0.8

Co-authored-by: Kevin Powick <kpowick@gmail.com>
Co-authored-by: Ian McGowan <ian.mcgowan@gmail.com>
Co-authored-by: Peter Schellenbach <petes@zumasys.com>
@kpowick kpowick removed this from the v2.0.8 milestone Jun 11, 2020
@dthiot
Copy link
Contributor

dthiot commented Jun 11, 2020 via email

@itsxallwater itsxallwater changed the title [FEATURE] [was bug] Label jumping out of code block [FEATURE] Label jumping out of code block Aug 11, 2020
itsxallwater added a commit that referenced this issue Sep 1, 2020
* RegEx fix for labels with leading spaces.

* 98-Issue linting with multiple levels of quoting

* RegEx improvement for quoted strings

* Replace parenthesis and quoted strings with empty ones.
- Removed repetitive Trim()s
- Removed unnecessary RegEx tests
- Simplified line split on semicolon due to above changes.
- Results: Faster execution. Better coverage for split on semicolon

* - Cleanup lines[] array at start so repeated test/replace not needed
- Removed unnecessary trim()s
- Removed boolean equality tests. if(x.test == true) vs if(x.test)
- Fixed GOTO/GOSUB when followed by more than one space GOTO <s><s> 100

* RegEx fix-ups to support recent changes

* Corrected comment

* Linter RegEx cleanup

* FOR/NEXT tracking improvements and updated error messages
- Nested For/Next statements now validated
- NEXT with no variable is now allowed
- One line per error: Removed extraneous, repeated diagnostic messages
- Reworded / Clarified some error messages for consistency

* Minor code clean-up

* Replace char-at-a-time split on ; with split(";")

* Maintain original character spacing for Intellisense error markers
- RegEx improvements and consistency between client and server
- Improved getWord() function

* Corrected indentation for ELSE END with trailing comment (issue #45)

* Corrects Label error with GOTO/GOSUB. Issue #99
- Also now allows dotted numeric labels such as GOTO 100.5

* Revert "Corrects Label error with GOTO/GOSUB. Issue #99"

This reverts commit 68b0166.

* Inspect TM Scopes->Inspect Editor Token and Scopes

* Fix error processing customWordPath when last
line is blank. May relate to #24.

* Changed GOTO/GOSUB scope jumping message from Error to Warning

* Numerous updates related to For/Next and Label tracking
- Issue #45: Alignment of If/Else
- Issue #96: Allow periods in labels
- Issue #99: Corrected Label error with GOTO/GOSUB.
- Issue #104: Corrected For/Next with leading label
- Issue #105: Changed GOTO scope jumping message from Error to Warning
- RegEx: Label comments to include "REM, *, and !"
- NEXT can be used without FOR variable.

* Shipping 2.0.7

* Re-introduce accidentally deleted /doc files

* Fixes issue 107: FOR/NEXT error when no whitespace after FOR variable.

* Shipping 2.0.8

* Issue #110: Do not change indentation of comment lines

* Issue #111: Added user setting to control indentation of comment lines.

* Bump elliptic from 6.5.2 to 6.5.3 (#119)

Bumps [elliptic](https://github.com/indutny/elliptic) from 6.5.2 to 6.5.3.
- [Release notes](https://github.com/indutny/elliptic/releases)
- [Commits](indutny/elliptic@v6.5.2...v6.5.3)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update Extension Guide.md

Updated AccuTerm connector and host program links.

* Feature request 113 (#121)

* Update package.json

Added MVBasic.customFunctionPath

* Update server.ts

added functionality to Load CustomFunction definition

* Add files via upload

* Update Extension Guide.md

added documentation for Custom functions and subroutines 6.8

* Update server.ts

Removed `data: functions[i].index,` for now since Microsoft has not responded yet.  It works without it, and it's no longer in the Microsoft API documentation.

* - Fix issue with startLoop/endLoop in server code parser
- Clean up unused variable in server
- Expand docs on new customFunctions feature

* Shipping 2.0.9

Co-authored-by: Kevin Powick <kpowick@gmail.com>
Co-authored-by: Ian McGowan <ian.mcgowan@gmail.com>
Co-authored-by: Peter Schellenbach <petes@zumasys.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Brent Blair <bblair@encompass.com>
itsxallwater added a commit that referenced this issue Sep 2, 2020
* RegEx fix for labels with leading spaces.

* 98-Issue linting with multiple levels of quoting

* RegEx improvement for quoted strings

* Replace parenthesis and quoted strings with empty ones.
- Removed repetitive Trim()s
- Removed unnecessary RegEx tests
- Simplified line split on semicolon due to above changes.
- Results: Faster execution. Better coverage for split on semicolon

* - Cleanup lines[] array at start so repeated test/replace not needed
- Removed unnecessary trim()s
- Removed boolean equality tests. if(x.test == true) vs if(x.test)
- Fixed GOTO/GOSUB when followed by more than one space GOTO <s><s> 100

* RegEx fix-ups to support recent changes

* Corrected comment

* Linter RegEx cleanup

* FOR/NEXT tracking improvements and updated error messages
- Nested For/Next statements now validated
- NEXT with no variable is now allowed
- One line per error: Removed extraneous, repeated diagnostic messages
- Reworded / Clarified some error messages for consistency

* Minor code clean-up

* Replace char-at-a-time split on ; with split(";")

* Maintain original character spacing for Intellisense error markers
- RegEx improvements and consistency between client and server
- Improved getWord() function

* Corrected indentation for ELSE END with trailing comment (issue #45)

* Corrects Label error with GOTO/GOSUB. Issue #99
- Also now allows dotted numeric labels such as GOTO 100.5

* Revert "Corrects Label error with GOTO/GOSUB. Issue #99"

This reverts commit 68b0166.

* Inspect TM Scopes->Inspect Editor Token and Scopes

* Fix error processing customWordPath when last
line is blank. May relate to #24.

* Changed GOTO/GOSUB scope jumping message from Error to Warning

* Numerous updates related to For/Next and Label tracking
- Issue #45: Alignment of If/Else
- Issue #96: Allow periods in labels
- Issue #99: Corrected Label error with GOTO/GOSUB.
- Issue #104: Corrected For/Next with leading label
- Issue #105: Changed GOTO scope jumping message from Error to Warning
- RegEx: Label comments to include "REM, *, and !"
- NEXT can be used without FOR variable.

* Shipping 2.0.7

* Re-introduce accidentally deleted /doc files

* Fixes issue 107: FOR/NEXT error when no whitespace after FOR variable.

* Shipping 2.0.8

* Issue #110: Do not change indentation of comment lines

* Issue #111: Added user setting to control indentation of comment lines.

* Bump elliptic from 6.5.2 to 6.5.3 (#119)

Bumps [elliptic](https://github.com/indutny/elliptic) from 6.5.2 to 6.5.3.
- [Release notes](https://github.com/indutny/elliptic/releases)
- [Commits](indutny/elliptic@v6.5.2...v6.5.3)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update Extension Guide.md

Updated AccuTerm connector and host program links.

* Feature request 113 (#121)

* Update package.json

Added MVBasic.customFunctionPath

* Update server.ts

added functionality to Load CustomFunction definition

* Add files via upload

* Update Extension Guide.md

added documentation for Custom functions and subroutines 6.8

* Update server.ts

Removed `data: functions[i].index,` for now since Microsoft has not responded yet.  It works without it, and it's no longer in the Microsoft API documentation.

* - Fix issue with startLoop/endLoop in server code parser
- Clean up unused variable in server
- Expand docs on new customFunctions feature

* Shipping 2.0.9

* Update server.ts

It looks like I left the line `customFunctionPath = settings.MVBasic.customFunctionPath;` out of the merge.

* Shipping v2.0.10

Co-authored-by: Kevin Powick <kpowick@gmail.com>
Co-authored-by: Ian McGowan <ian.mcgowan@gmail.com>
Co-authored-by: Peter Schellenbach <petes@zumasys.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Brent Blair <bblair@encompass.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants