rokucommunity / brighterscript

A superset of Roku's BrightScript language
MIT License
160 stars 46 forks source link

inconsistent renaming of namespaced objects #817

Closed ZeeD closed 1 year ago

ZeeD commented 1 year ago

Note: I was looking at some way to integrate my brighterscript-based project with roca. During some tests, I stumbled in a strange issue (that normally doesn't occur to me). To better explain my scenario, on https://github.com/ZeeD/brs_roca I've created a demo repo.

My goal is to be able to write the roca tests as .bs, and to be able to call the various functionalities I have developed. One example of these tests is https://github.com/ZeeD/brs_roca/blob/main/tests/foo/example.test.bs .

On https://github.com/ZeeD/brs_roca/blob/main/dist/tests/foo/example.test.brs you can see the compiled sources. While, on https://github.com/ZeeD/brs_roca/blob/main/dist/tests/foo/example.test.brs#L6 ns.C is correctly replaced with ns_C, this doesn't happen for ns.f and ns.s.

is this a problem of my configuration? a bug of the bsc compiler?

TwitchBronBron commented 1 year ago

The brighterscript validator and transpiler depends on scopes for namespace discovery. All files in brighterscript need to be included in a scope in order for them to function properly. Files nested under pkg:/source are all included automatically in the 'source' scope. Then, all other files should be explicitly included into a scope via components in some fashion.

Namespace transpiling works by discovering a DottedGetExpression (like ns.f) and asking the scope if ns is a namespace. In this situation, your ./tests files aren't included in any scope because it's not in the source/ folder and it's not included by any component. BrighterScript tried to warn you about this with diagnostic code 1013 but it was disabled in the bsconfig.json

Normally I would suggest moving all these tests into the source/ folder, but roca doesn't support that. So, with the way BrighterScript works right now, you'll need to make sure all your test files are included in at least one component. Perhaps something like this:

components/TestComponent.xml

<component name="TestComponent">
    <script uri="TestComponent.brs"
</component>

components/TestComponent.bs

import "pkg:/tests/example.test.bs"
' you'll need to add an import line for every other *.test.bs file

Also, I would advise against blanket-disabling diagnostic 1002. Instead, just disable it on the roca() line (hopefully one way we'll support d.bs files that can declare functions like that). Like this:

    return roca(args).describe("ns", sub() 'bs:disable-line 1002

I realize this is a little counter-intuitive, but hopefully this will help get you unblocked.

It seems to work great for me locally now: image

image

image

ZeeD commented 1 year ago

thanks for the explanation and the solution. I would like to add only one thing: from what I understand roca requires a main function per "file" / "test suite". So I needed to add another annotation to tell bsc to allow multiple main functions in my project

TwitchBronBron commented 1 year ago

Yeah, so you'll need to add a 'bs:disable-line for each of those as well at the moment, unfortunately. It's a byproduct of my "component to import them all" hack.

I'm definitely open to figuring out how to make this process easier. Just not sure quite what the right answer is.

ZeeD commented 1 year ago

Uhm, upon some additional testing, I think I'll abandon the usage of roca (and brs, I think).

While I acknowledge their impressive features (brs is basically a full brightscript interpreter!) they lack the support for some core component I need to interact with in my project...

So I think we can close this issue, as at least for me I will not need any specific feature anymore from brigherscript.