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

Used GenState to store variable scope #3899

Merged
merged 11 commits into from
Aug 19, 2024
Merged

Used GenState to store variable scope #3899

merged 11 commits into from
Aug 19, 2024

Conversation

B-rando1
Copy link
Collaborator

As discussed in #3888, it is better to store the scope in GenState than to pass it around all over drasil-code. I modified GenState to hold that information. I also made sure that the currentScope value is being modified whenever drasil-code goes into a specific situation. Lastly, I used currentScope to give variables the proper scope when they are created.

I have a few notes:

  • Just double-checking - when we go back from a function call, does the State automatically revert its values back? E.g. if a generator function in the global scope calls a generator function in the local state, when the function terminates will it automatically set the state back to global?
  • It is possible that I made mistakes when telling it when to set the currentScope value. I based it on what I had in Finished tracing out scope to its sources in drasil-code #3888, which is at least a good baseline, though.
  • This current method of scoping assumes that variables belong to the current scope. If it creates a variable in a global scope, then references it in a local scope, it will check GenState, see that it's in a local state, and say the variable was created in a local scope. This is exactly the kind of behaviour we're trying to prevent, so I think we still need a bit more here.
    • I don't think this work is a waste either, though. Whatever the 'bit more' will be, we'll need to know the current scope when creating new variables.

These last two points show the need for a better way of testing the scope of a variable in generated code. We should be able to do this once #3891 is merged, by temporarily modifying one of the renderers to display a variable's scope.

This will allow us to store the scope in the state instead of passing it
around explicitly.
If I am correct and also didn't miss anything, the scope should now be
accurate throughout `drasil-code`.
@JacquesCarette
Copy link
Owner

when we go back from a function call, does the State automatically revert its values back?

Yes

If it creates a variable in a global scope, then references it in a local scope, it will check GenState, see that it's in a local state, and say the variable was created in a local scope.

I don't fully understand this. The code should be able to look at the variable itself and query its scope, no?

@B-rando1
Copy link
Collaborator Author

I don't fully understand this. The code should be able to look at the variable itself and query its scope, no?

Hmm, I'm trying to think. I don't think we're currently keeping track of what scope is tied to a variable.

  • The issue is, I think, that in drasil-code, every time we need to reference a variable, a 'new' variable is created - the name, type, scope etc. are only the same if we keep track of enough information to make sure they're the same. The current changes in this PR give us enough information to know what scope is currently being generated, but not enough to know what scope a variable was originally created under.
    • We could add to GenState to keep a map from variable name to its scope.
    • We could see if there's a way of only creating a variable once in drasil-code, and using the exact same variable everywhere it's needed. I'm just not sure if that's feasible though - I need to think some more to give a concrete example, but I think there are many places where the use of a variable automatically handled the translation process.
    • We could change drasil-gool so that only declaring variables requires a scope, and the rest of the time scope is implicit. This is a new idea that I haven't thought of before now, but it might feel more like normal code. It would also get rid of this issue. The only thing is that we would need to add to drasil-gool to keep track of a variable's scope on that end. It might be worth thinking about, though.

I'm not sure if I've completely thought through our options, so I might have to make another comment on #3821 tomorrow. Let me know if you have any thoughts here.

We're planning to move it at this point.
And propogated the function calls down as far as I could without adding
to the AST.
And integrated throughout GOOL
These changes only affect Julia.  There are two places they take place:
- When a global constant is declared, the `const` keyword is used.
- When a global variable is assigned to, the `global` keyword is used to
  explicitly declare that it is a global variable.

Notes:
- Right now, the `global` keyword is added way too often.  Unfortunately
  this won't be easy to fix, but we should fix it eventually.
- Because Julia's implementation of `multiAssign` creates an empty
  variable, I had to add a hack to accomodate it.
@B-rando1
Copy link
Collaborator Author

B-rando1 commented Aug 14, 2024

After talking about it a lot, I decided to just go for the changes I thought we needed. Going off my comment in #3821, this implements option 2. It moves a lot of the weight of keeping track of scope from drasil-code to drasil-gool, which I think should more safely address the concerns raised above.

Just a few notes:

  • This changes GOOL's variable API a fair bit - instead of requiring a variable's scope every time it is used, its scope is only needed when it is declared. After that, it is stored by GOOL.
  • As I've mentioned before, until we give GOOL more information about the scope currently being generated, the Julia renderer needs to be super overkill about its global declarations. It works, but it's not the nicest-looking Julia code ever.
  • Because of how Julia's multiAssign is implemented, I needed to add a hack to say that a variable with an empty name has a local scope.

As I mentioned at the end of this comment, another benefit of doing this as an intermediate step is that it removes some work in the wrong direction that had unfortunately made it into main.

There are a few macros that involve creating a temporary variable.
Previously I just set them to be local, but now they use the scope of
the input variable to set their own scope.

This has a possibility of being `global` more often than it should, but
it should be more accurate than it was.
@JacquesCarette JacquesCarette merged commit 0b4fa96 into main Aug 19, 2024
5 checks passed
@JacquesCarette JacquesCarette deleted the StatefulScope branch August 19, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants