Same name of variable and subroutine becomes syntax error

[nonkit]

Following sample code causes syntax error in SBO v1.0.

test = "Hello"
test()
Sub test
  TextWindow.WriteLine(test)
EndSub

[PieterBenjamin]

Ah, I think I see. It looks like SB does shadow variables correctly but in this instance, in the snippet test = "Hello" the name test is not being treated as the name of a variable but a subroutine call. This does seem incorrect for a few reasons

1. I think the only way to call a subroutine in SB is with parentheses so it does beg the question why there would ever be a conflict when looking up the name of methods vs variables?
2. If there WERE to be an issue here, we would expect it to be a naming issue, but in reality it looks to be an issue with trying to call a subroutine.

I think we may be able to get around this by adding a special sequence of characters to the end (or beginning) of subroutines as a sort of flag so when writing to the environment and looking the values up we never make a mistake.

However, this sounds like a pretty substantial design choice and I'm not sure which way to go. @OmarTawfik could you drop your opinion in the ring?

[PieterBenjamin]

As an update, this works:

for test = 1 to 10
    TextWindow.WriteLine(1)
EndFor

test()
Sub test
    TextWindow.WriteLine(1)
EndSub

but this does not:

for test = 1 to 10
    TextWindow.WriteLine(test)
EndFor

test()
Sub test
    TextWindow.WriteLine(1)
EndSub

and of course anything with TextWindow.WriteLine(test) doesn't work

[OmarTawfik]

I think you just discovered a bug in the binder :) To correct this, do you think we should allow shadowing? IMO, it just teaches users bad practices, and we should disallow it consistently, but I leave this up for discussion. Please let me know if you have thoughts on it.

[PieterBenjamin]

@OmarTawfik to be honest I think the issue here is with the parser treating an expression as a value - shadowing seems to have already been decided for the language and (I think) is performing incorrectly here. Do you think backend/compiler/parser.cs is a good place to start looking?

[nonkit]

I also reported this as a parity issue because SBD and SBO v0.91 allow shadowing. But my opinion is that shadowing is not so important for programming learners.

[OmarTawfik]

@PieterBenjamin my hypothesis was that after parsing is done, there is a single method (below) that is "binding" all random identifiers to either: 1) a library. 2) a submodule. 3) a variable.

Since collecting sub-module names happens first, we automatically assume test is a sub-module, and therefore it cannot be assigned to or printed to the text window.

https://github.com/sb/smallbasic-editor/blob/fcd1eb22bf67b2756c993937bfd5e945a74756de/Source/SmallBasic.Compiler/Binding/Binder.cs#L530-L560

To verify if that is the case, I suggest starting by:

1) adding a test like the one I pasted below, and put a break point in the binder method to see if it was so. 2) the fix might be as easy as if expectsValue was true, we can just return an identifier and not check sub-modules. 3) add a few tests to verify corner cases, and make sure the existing tests also pass.

https://github.com/sb/smallbasic-editor/blob/fcd1eb22bf67b2756c993937bfd5e945a74756de/Source/SmallBasic.Tests/Runtime/StatementsTests.cs#L235-L252

[PieterBenjamin]

@OmarTawfik sorry for the long silence, had some assorted business on my end to deal with.

I tried reordering the binding method as you suggested to the following:

private BaseBoundExpression BindIdentifierExpression(IdentifierExpressionSyntax syntax, bool expectsValue)
 {
    bool hasErrors = false;
    string name = syntax.IdentifierToken.Text;

     if (Libraries.Types.TryGetValue(name, out Library library))
     {
          if (expectsValue)
          {
                hasErrors = true;
                this.diagnostics.ReportExpectedExpressionWithAValue(syntax.Range);
           }

            return new BoundLibraryTypeExpression(syntax, hasValue: false, hasErrors, name);
     }
     else if (expectsValue)
     {
          return new BoundVariableExpression(syntax, hasValue: true, hasErrors, name);
     }
     else 
     {
          if (expectsValue)
          {
               hasErrors = true;
               this.diagnostics.ReportExpectedExpressionWithAValue(syntax.Range);
          }

          return new BoundSubModuleExpression(syntax, hasValue: false, hasErrors, name);
     } 
}

Note: all I did was move a check for expectsValue before the previous check else if (this.definedSubModules.Contains(name))

And that seems to have solved it from my basic testing, but I'm not sure how to write tests on the backend. I'll get working on that and let you know when I've made some progress.

Could you explain why this appears to be the fix? The following code segment works as expected on my version, but won't compile on the live super basic

test()
for test = 1 to 10
    TextWindow.WriteLine(test)
EndFor

test()
Sub test
    TextWindow.WriteLine("hello world!")
EndSub
test()

I guess my main confusion is I'm unsure what the Binder does. It sounds like it would set the behavior for a keyword across the program but the above code would disagree with that.

[PieterBenjamin]

As a quick update, I've figured out how to add tests. Would you like any explanatory comments added for each test?

Also, this is definitely a good time to revisit the shadowing issue. I think it's a decent crutch for beginners, but I'd love to have a discussion on it.

[OmarTawfik]

@PieterBenjamin thanks for looking into it! to answer your questions:

• that change might not fix the intended issue, because we should not be returning a BoundSubModuleExpression if it wasn't defined, that will just lead to confusing error messages. One valid order would be:
  1. if it was a library -> return a library object
  2. if it was a submodule -> if you expect a value, return a variable, otherwise, return a submodule.
  3. otherwise, return a variable.
  4. keep all reported errors the same.
• you don't need to add comments to tests if you don't think one is needed. You might want to add this issues url in a comment though :)
• to answer your question about what the binder does, basically a typical compiler pipeline will do the following:
  1. phase 1: scanner takes a string, and produces a list of tokens. these can be things like IF_TOKEN, IDENTIFIER, ELSE_IF_TOKEN, etc...
  2. phase 2: parser takes a list of tokens, and produces a tree of syntax nodes. the nodes are dumb and try to minimize the checks done here. For example, if it sees x = Math.Foo() it will bind it into an assignment expression, without actually checking if Foo exists.
  3. phase 3: the binder takes the syntax tree, and produces a bound tree. It does all the extra checking, like making sure you supply the correct number of arguments to methods, call existing sub-modules, assign things in the right order, etc...
• the reason most compilers try to move all possible checks from parsing to binding, is that producing a complete syntax tree (even from invalid code) is crucial to support IDE features like auto-completion or hover tooltips. If the parser breaks because it cannot find Foo, the IDE won't be able to use the resulting tree to give you intellisense in a different part of the file.

[PieterBenjamin]

@OmarTawfik I think I got it this time:

else if (this.definedSubModules.Contains(name))
{
    if (expectsValue)
    {
        return new BoundVariableExpression(syntax, hasValue: true, hasErrors, name);
    }

    return new BoundSubModuleExpression(syntax, hasValue: false, hasErrors, name);
}

It's also worth noting that the hasErrors variable is somewhat unnecessary at this point. If this looks like a proper fix, I can whip up some tests and submit a pr

edit: I'll have to change the expected output for the ItReportsAssigningToSubModule test. In the current version, this would be considered an error but with the changes I believe this would be correct behavior. Is it alright if I change that test?

[OmarTawfik]

@PieterBenjamin that would be great! Please do 😄

[PieterBenjamin]

@OmarTawfik created a (hopefully successful) pull request. The changes were pretty minimal so I'm sorry I didn't get this all done sooner!