Started adding `Global` vs `Local` scope distinction #3838

B-rando1 · 2024-07-08T15:36:36Z

Main changes:

I renamed the existing ScopeSym and all its derivatives to VisibilitySym, because it has to do with public vs private class members. Hopefully this isn't too confusing...
I created a new ScopeSym that deals with global vs local variables.
I added Scope as a new parameter to var, staticVar, extVar, and const.

Notes:

Currently none of the renderers do anything with this, as they don't need to. Julia will need it, though.
To make our current code work with the changes, I had to give a scope to variables, constants, etc. Because I wanted to get the code working again as soon as possible, I set it to local in every instance. I might regret this later, but I just haven't thought through what we need to do to give the proper value in each case.
The order of the parameters for var, etc. could be up for debate. Currently the Scope is the last parameter for these. I think that makes sense, as it feels less important than the name or type of the variable. As @JacquesCarette's comment shows, however, putting it nearer the start of the list of parameters would make certain internal functions look simpler.
- I know there are several previous instances (e.g. objMethodCall) where we use a smart constructor to rearrange the order of parameters from a user-friendly ordering to an internal-friendly ordering. Would this be a good place to do that?

Contributes towards #3821

JacquesCarette

Just one thing to change, otherwise this looks great.

JacquesCarette · 2024-07-08T15:57:16Z

code/drasil-code/test/HelloWorld.hs

-  varDec $ var "a" int,
-  varDecDef (var "b" int) (litInt 5),
-  listDecDef (var "myOtherList" (listType double)) [litDouble 1.0,
+  varDec $ var "a" int local,


what would make more sense would be to introduce a new combinator locvar that is a synonym for var ... local and use that instead. Everywhere. It just 'clicked' with me when I was reviewing this part.

Good idea! I'll add that in.

While we're talking about smart constructors, do you think we should change the var in the typeclass to var', and change the order of its parameters? As you guessed, the fix for the type error in the C++ renderer (as well as a few places in drasil-code) would look nicer if we were to do that. This is the current fix, for reference:

Drasil/code/drasil-gool/lib/GOOL/Drasil/LanguageRenderer/CppRenderer.hs

Lines 275 to 278 in fbfc58e

var n t s = pair1 (\tp -> var n tp (pfst s)) (\tp -> var n tp (psnd s)) t

staticVar n t s = pair1 (\tp -> staticVar n tp (pfst s)) (\tp -> staticVar n tp (psnd s)) t

constant n t s = pair1 (\tp -> constant n tp (pfst s)) (\tp -> constant n tp (psnd s)) t

extVar l n t s = pair1 (\tp -> extVar l n tp (pfst s)) (\tp -> extVar l n tp (psnd s)) t

If we changed the operand order, we could get rid of the lambdas and leave the functions partially applied, as they were before.

Yes, changing the order of parameters to make things smooth is worthwhile.

And many similar changes. Basically, there are almost no occurrences left of `scope` in either of `drasil-gool` or `drasil-code`.

`drasil-code` doesn't work with it yet, but one step at a time. Also, the C++ renderer has some weird type errors. I'll hopefully be able to fix it and amend this commit message, but if not, then you'll see this message :).

None of our current renderers do anything with it, and for now I made every variable `local`, since I'm not quite clear on how to do more than that yet. It works though, and I _think_ it's roughly in the right direction.

That was quite the merge conflict! It should all be working now though. I also created the `locvar` smart constructor for local variables. I'll have to put it to use tomorrow, though.

I changed the order of the parameters for `var`, `constant`, `extVar`, and `staticVar`. I then added and integrated some smart constructors to make using them simpler.

B-rando1 · 2024-07-09T16:01:44Z

I think we should find a better solution to my comment before we go further with this work.

I realized that `extVar`s are always `global` and `staticVar`s are always local, so they don't need that parameter.

This will represent the `main` function of a module, which is either global or local, depending on the language.

- I updated everything in `drasil-code/test` to use the proper scope. - I also started getting `drasil-code` to use the proper scope. It's slow going, but I fixed what I'm hoping was one of the trickier functions that needed a scope.

Similar to `locVar`, it makes it easier to create a variable in the 'main function' scope. I also renamed `locvar` to `locVar`, to be more consistent with `extVar` and `staticVar`.

I pushed it back to `mkVar` and `mkVal`. These get called by a ton of other things, but _hopefully_ I'm getting close to the end.

This one feels like a step backwards, as I created more `TODO`'s than I removed, but it's a necessary step.

B-rando1 requested review from JacquesCarette and bmaclach as code owners July 8, 2024 15:36

JacquesCarette requested changes Jul 8, 2024

View reviewed changes

B-rando1 added 4 commits July 8, 2024 14:44

Renamed ScopeSym to VisibilitySym

04b02d1

And many similar changes. Basically, there are almost no occurrences left of `scope` in either of `drasil-gool` or `drasil-code`.

Updated drasil-gool to use the new ScopeSym

a05f113

`drasil-code` doesn't work with it yet, but one step at a time. Also, the C++ renderer has some weird type errors. I'll hopefully be able to fix it and amend this commit message, but if not, then you'll see this message :).

Finished adding the new ScopeSym

afb158d

None of our current renderers do anything with it, and for now I made every variable `local`, since I'm not quite clear on how to do more than that yet. It works though, and I _think_ it's roughly in the right direction.

Rebased onto main

d8fd41e

That was quite the merge conflict! It should all be working now though. I also created the `locvar` smart constructor for local variables. I'll have to put it to use tomorrow, though.

B-rando1 force-pushed the GlobalLocalScope branch from fbfc58e to d8fd41e Compare July 8, 2024 20:52

Updated the definitions and use of VariableSym

870ecb5

I changed the order of the parameters for `var`, `constant`, `extVar`, and `staticVar`. I then added and integrated some smart constructors to make using them simpler.

B-rando1 marked this pull request as draft July 9, 2024 16:00

B-rando1 added 5 commits July 12, 2024 11:42

Removed Scope parameter from extVar and staticVar

5353a7e

I realized that `extVar`s are always `global` and `staticVar`s are always local, so they don't need that parameter.

Added mainFn to set of ScopeSym methods

7f55716

This will represent the `main` function of a module, which is either global or local, depending on the language.

Started integrating mainFn into GOOL code

588147a

- I updated everything in `drasil-code/test` to use the proper scope. - I also started getting `drasil-code` to use the proper scope. It's slow going, but I fixed what I'm hoping was one of the trickier functions that needed a scope.

Merge branch 'main' into GlobalLocalScope

95fc057

Found proper scope for more variables

baf3f56

B-rando1 force-pushed the GlobalLocalScope branch from 3b6f656 to baf3f56 Compare July 15, 2024 13:50

B-rando1 marked this pull request as ready for review July 15, 2024 13:51

B-rando1 added 3 commits July 17, 2024 16:11

Added the mainVar smart constructor

03e68cc

Similar to `locVar`, it makes it easier to create a variable in the 'main function' scope. I also renamed `locvar` to `locVar`, to be more consistent with `extVar` and `staticVar`.

Made some more progress assigning correct scope

11862a6

I pushed it back to `mkVar` and `mkVal`. These get called by a ton of other things, but _hopefully_ I'm getting close to the end.

Continued pushing scope higher into drasil-code

b313e44

This one feels like a step backwards, as I created more `TODO`'s than I removed, but it's a necessary step.

B-rando1 mentioned this pull request Jul 19, 2024

Drasil Research Group Meeting, 3pm, Monday July 22, 2024, ITB/225 #3852

Closed

Merge branch 'main' into GlobalLocalScope

108b8a9

JacquesCarette approved these changes Jul 21, 2024

View reviewed changes

JacquesCarette merged commit ce525a2 into main Jul 21, 2024
5 checks passed

JacquesCarette deleted the GlobalLocalScope branch July 21, 2024 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Started adding `Global` vs `Local` scope distinction #3838

Started adding `Global` vs `Local` scope distinction #3838

B-rando1 commented Jul 8, 2024

JacquesCarette left a comment

JacquesCarette Jul 8, 2024

B-rando1 Jul 8, 2024

JacquesCarette Jul 9, 2024

B-rando1 commented Jul 9, 2024

	var n t s = pair1 (\tp -> var n tp (pfst s)) (\tp -> var n tp (psnd s)) t
	staticVar n t s = pair1 (\tp -> staticVar n tp (pfst s)) (\tp -> staticVar n tp (psnd s)) t
	constant n t s = pair1 (\tp -> constant n tp (pfst s)) (\tp -> constant n tp (psnd s)) t
	extVar l n t s = pair1 (\tp -> extVar l n tp (pfst s)) (\tp -> extVar l n tp (psnd s)) t

Started adding Global vs Local scope distinction #3838

Started adding Global vs Local scope distinction #3838

Conversation

B-rando1 commented Jul 8, 2024

Main changes:

Notes:

JacquesCarette left a comment

Choose a reason for hiding this comment

JacquesCarette Jul 8, 2024

Choose a reason for hiding this comment

B-rando1 Jul 8, 2024

Choose a reason for hiding this comment

JacquesCarette Jul 9, 2024

Choose a reason for hiding this comment

B-rando1 commented Jul 9, 2024

Started adding `Global` vs `Local` scope distinction #3838

Started adding `Global` vs `Local` scope distinction #3838