Conversation
|
This sounds reasonable, with your proposition the u/r status never changes, so the system is easier to think about. Question: why is there a distinction between |
|
I had understood from your explanation that builtin variables are evaluated on demand:
I wasn't able to find this in the ocamltest source. My understanding is that this is in fact not the case: actions will typically define some of the builtin variables that are relevant, and then the If my intuition above is correct, then I think that a clearer mental model is as follows:
The code prevents user from defining via I agree that reserving variables when they are first set is a bug in trunk, because it prevents setting them in parallel branches. |
ocamltest/tsl_semantics.ml
Outdated
| let var = Variables.find_or_make variable_name in | ||
| match (Variables.find_variable variable_name, | ||
| Environments.is_variable_defined var env, | ||
| decl) |
There was a problem hiding this comment.
This is obscure to me (for example: if we just called find_or_make, why are we calling find again?).
Given your explanation in the PR, taking into account the new logic of never registering user variables, I think that the meaning of each component is as follows:
Variables.find_variabletells us whether a variable is reserved (but it may or may not be set)Environments.is_variable_definedtests if the variable is actually defined (notabs, in your terminology)
This is actually a case where labeled tuples could be put to good use (untested)!
let reserved = Option.is_some (Variables.find_variable variable_name) in
let defined = Environments.is_variable_defined var env in
match (~reserved, ~defined, ~decl) with
| (~reserved:false, ~defined:false, ~decl:true) ->
Environments.add var value env
| (~reserved:_, ~defined:_, ~decl:true) ->
raise (Variables.Variable_already_registered variable_name)
| (~reserved:false, ~defined:false, ~decl:false) ->
raise (Variables.No_such_variable variable_name)
| (~reserved:_, ~defined:_, ~decl:false) ->
Environments.add var value envOr a simpler approach:
let reserved = Option.is_some (Variables.find_variable variable_name) in
let defined = Environments.is_variable_defined var env in
if decl then begin
if reserved || defined then raise (Variables.Variable_already_registered variable_name)
end else begin
if not (reserved || defined) then raise (Variables.No_such_variable variable_name)
end;
Environments.add var value envThere was a problem hiding this comment.
I've changed this function along the lines of your simpler approach, and I changed the name find_or_make to from_name to make it less confusing.
This is important. The environment is not an environment, but an environment transformer, which will be applied to the environment inherited from the OS. If a variable is Here's your table:
|
If you look in
In fact, you cannot |
cc1e15b to
1d3252f
Compare
How it works before this PR
Variables are handled with two data structures:
The map is persistent and handled as expected in a stack-like fashion following the tree structure of the test script. The global hash table, on the other hand, is a mutable data structure where variables are added but never removed.
A variable has two possible states in the global table: registered or unregistered.
In the environment, a variable has three possible states:
The possible states of a variable are:
r/strr/nulr/absu/absu/nulThere are 4 operations that a test script can do on a variable:
set,=,+=,unset. They change a variale's state as follows (where * denotes an error).set:r/str-> *r/nul-> *r/abs-> *u/nul->r/stru/abs->r/str=, +=:
r/str->r/strr/nul->r/strr/abs->r/stru/abs-> *u/nul-> *unset:
r/str->r/nulr/nul->r/nulr/abs->r/nulu/abs->u/nulu/nul->u/nulThere are two kinds of variables: built-in variables and user variables. Built-in variables are preloaded in the global hash table, while user variables are inserted on demand by
set. If a built-in variable is used while absent from the environment, it will get a default value, computed at the time of use.The problem is that the state
r/absrepresents two things:setcannot be used on these.setshould be usable on these variables, but it isn't.The fix
If we stop registering user variables, we remove the problem of user variables that were registered in another branch of the script. We get
u/stras a new possible state and the following transitions:set:r/str-> *r/nul-> *r/abs-> *u/abs->u/stru/nul-> *u/str-> *=,+=:r/str->r/strr/nul->r/strr/abs->r/stru/abs-> *u/nul->u/stru/str->u/strunset:r/str->r/nulr/nul->r/nulr/abs->r/nulu/abs->u/nulu/nul->u/nulu/str->u/nulNote: I removed the set:
u/nul->u/settransition that allowed the user toseta user variable that was alreadyunset; it is quite clear from the code that it was a bug. The intent was that a variable should be initiallysetorunset, then modified with=,+=, orunset.Alternate design
A more proper fix would be to explicitly insert the built-in variables in the environment (as a 4th state, "default") and make the primitives act purely on the environment. This would be a more invasive fix, with a greater change of breakage.
fixes #13941