Allow nested namespaces

[elliot-nelson]

SUMMARY

Allow nested namespaces (fixes #624).

DETAILS

Disclaimer - this is my first go-round in the codebase, so if I've everything I've done here is totally wrong, don't hesitate to say so! Any feedback appreciated.

Here's the problems I attempted to solve:

1. We need to unblock namespace statements, but, they aren't generically allowed -- they can't be inside a function or a class, for example. To accomplish this I invented a new concept called a ParserScope, kept on a stack, that tries to express the stack of namespaces, classes, and functions below us. As long as the stack consists only of namespaces, it's OK to declare a namespace.

2. Once you have declared a namespace, you need a way to fully qualify the name to include your parent namespaces. The way I've chosen to do this is determine the set of parent name expressions at parse time -- we have the stack, so we can grab the name expressions of all containing namespaces and hold onto them in the namespace statement, consuming them at the appropriate time (when we know whether it's brightscript or brighterscript mode).

3. Last, we need a diagnostic adjustment. I chose to leave the root-level diagnostic alone (there's no callers anymore, but there might be one in the future), and invent a new one for the "root- or namespace-level" requirement.

POSSIBLE ALTERNATIVES

I am most hesitant about the code for problem (2). I actually didn't need to do this to get it working; I could create the NamespaceStatement as normal, and have the NamespaceStatement recursively walk up the findAncestor tree at getName() time to construct the qualified name.

What I didn't like about that approach is that running the compiler on my sample program produced the right output, but the unit tests gave me a lot of grief -- I kept running the parser, examining the namespace statement references, and all those statements were divorced of any tree, so the names were wrong (unqualified), even though the code generated the right output. (Maybe this is expected and I'm testing the wrong thing here; if so, perhaps we could swap implementation of (2) to walk ancestors instead, since I see that pattern in a couple other places in Statements.ts).

TESTING

• Ran locally on a sample program with a few namespaces and methods, produced the expected output.
• Added one new test, adjusted one expected test.
• Adjusted one unexpected test -- it seems like there was a syntax error in a test regarding completions, which I'm not sure was intentional or not, but had to remove to get the test passing.

[elliot-nelson]

Update... as I think about this more, if the output of Parser is an AST then my approach is definitely wrong because you want to be able to clip and paste nodes around in the tree and have their output be correct.

So I think I only need advice on the unit test -- maybe the Parser test just confirms that Level3 is named "Level3", and there's a spot for a higher level unit test that confirms that program produces output function "Level1_Level2_Level3_main".

[elsassph]

Hey @elliot-nelson, thanks for your contribution!

Yes from an AST point of view I'm not sure this is right. Maybe you could follow how classes build their qualified name (getName) by walking the AST using the findAncestor method.

[TwitchBronBron]

I plan on fully reviewing this PR tomorrow when I'm back to work, but I agree with @elsassph . These need to follow the same pattern as classes (using .findAncestor). This is because we need to support plugins that manipulate the AST, and so doing this in the parser is far too early in the process and would break AST modifications by plugins.

The .parent property is set during onProgramValidate, so any unit tests will need to call program.validate() before building/verifying the fully qualified name.

[TwitchBronBron]

Hey, thanks for your work on this feature! I really appreciate how focused you kept the PR. Your in-depth explanation really helped understand how and why you made your design decisions.

I have requested a few changes that better align this implementation with the design of the project.

The big thing is, I think you need to eliminate the ParserScope concept entirely. brighterscript plugins can modify the AST at several points within the project lifecycle, so your current implementation would prevent plugins from creating/editing/deleting namespace statements effectively because it would tie the AST to the original structure as found in the source files.

The correct way to handle this is to use the .findAncestor() ast functionality. As you already noticed, .parent isn't immediately available to ast nodes. Instead, the .parent property is set in the onFileValidate hook for every file. This means that several of the namespace tests may need modified to use program.setFile() and then program.validate() before checking the fully-qualified namespace name.

[elliot-nelson]

Thanks all for the feedback, based on comments I have:

• Cut the ParserScope stuff
• Updated NamespaceStatement to walk its ancestors to get the full dotted namespace name
• The test for Parser just confirms you can create nested namespaces
• The test for Statement now confirms that a NamespaceStatement retrieves its full dotted name, if in a program
• The test for BrsFileValidator now confirms that a namespace can't be declared in the wrong place
• Updated namespace docs to add the new allowed syntax

(The test for BrsFileValidator could be more robust, but I tested out putting a namespace in other places e.g. inside a class, in the middle of an expression, as a parameter to a function call, and all of that stuff is a syntax error for other reasons, so I think it can be left simple for now.)

[TwitchBronBron]

Looks great! I have two small suggestions in your unit tests that will improve error reporting if the tests were to ever start failing, but then this will be good to go!

[elliot-nelson]

Thanks!

• Updated BrsFileValidator tests to use diagnostic expectations.

[TwitchBronBron]

Looks great! Thanks for this.