Make spago install work when more advanced Dhall expressions are used
#815
Conversation
Control flow before this commit would do all the lookup work and get the needed values, only to fail if the packages that are trying to be added don't exist in the package set. This commit does the check first.
|
Ok. The failing test now passes. I'm not sure whether the change I made will affect other things or not. |
|
Looks like this PR will also need to be able to handle parsing of
|
In this commit, I moved the code that checks whether the user-requested packges exist in the package set into the `withRawConfigAST`, so that the new `Spago.Config.AST` module doesn't introduce a cyclial dependency with the `Spago.Config` module. I also provide both the raw and normalized AST expressions as it's the only way to correctly determine which packages haven't yet been installed.
|
CI fails with only one failure. test/SpagoSpec.hs:273:72:
1) Spago, spago install, Spago should not change the alternative config if it does not change dependencies
expected: "[warn] Failed to add dependencies. You should have a record with the `dependencies` key for this to work.\n[warn] Configuration file was not updated.\n"
but got: "[warn] Failed to add dependencies.\n\nExpected a top-level expression that produces a record with a `dependencies` key.\n\nValid top-level expressions are:\n - RecordLit - `{ key = value, ... }`\n - Let - `let binding = value in expr`\n - Prefer - `expr1 // expr2`\n - With - `expr1 with expr2`\n - Field - `expr.value`\n\nInvalid top-level expressions include but are not limited to: \n - Lam - \955(x : A) -> x (doesn't produce a Record-like expression)\n - App - `(\955(x : A) -> x) 4` (it's safer to let the user handle these unique cases)\n - Project - `{ key1 = value1, key2 = value2}.{ key }` (less frequently used)\n\nTop-level expression was: ./spago.dhall\n[warn] Failed to add dependencies.\n[warn] Configuration file was not updated.\n"
To rerun use: --match "/Spago/spago install/Spago should not change the alternative config if it does not change dependencies/"
Randomized with seed 642167733
Finished in 514.7330 seconds
110 examples, 1 failure |
|
Ok, the reason why this last test fails is because the root-level expression is a In this situation, I guess we do this? -- before
./spago.dhall // { sources = ["foo"] }
-- after
let config = ./spago.dhall
in config // { sources = ["foo"], dependencies = config.dependencies # ["new"] } |
|
@JordanMartinez I wouldn't like to edit user configs in such depth, so I think in this case it's fine to just fail? |
|
I can remove the detailed error message, but I still think this case should be supported because the failure is unexpected. Moreover, |
|
As a sanity check, I think the returned expression should be normalized to verify that the AST modification did not produce an invalid configuration file. Then we fail if it produces an invalid file but continue if it's still valid. |
If we are updating the dependencies field,
and have multiple let bindings
(e.g. dependencies = let x = ... let y = x let z = y in z),
then we should update the x binding's value.
If that value refers to a binding outside of the
scope of the dependencies key, then we stop
and do the update within x binding value.
Otherwise, we might introduce a side-effect in case
the x binding's value is used elsewhere in the expression.
Normally, we would use an Int to track the
scope depth of the let bindings.
In other words, `x` above would be 0, `y` would be `1` and
`z` would be `2`. When we reach `z`, we go back to `y` because `z`'s scope depth is not 0.
When we reach `y`, we got back to `x` becaues `y`'s scope depth is not 0.
Because `x'`s scope depth is 0, we stop and do the update.
However, if we have an expression like
dependencies = let x = { deps = [] } in x.deps
then we also need to keep track of a new key stack
that we only discover once we are WithinField.
Thus, rather than using `0`, we'll use `[Text]`.
An empty list corresponds to the 0 scope depth,
and a non-empty list means we can traverse back up
the expression if needed
This change enables us to pass the keystack back up to the let binding expression that defines the value for the binding. However, since the key stack might be empty, we can no longer use `NonEmpty Text`. Unfortunately, this change means `SearchingforField`'s arg must also be updated and by implication, `WithinField` and `SearchingForField` are conceptually the same thing.
To maintain the previous behavior, a let binding should only allow a `VariableName` to propagate to its parent expression if its own level is not an empty key stack (which means we're at the field we need to update). Otherwise, we will traverse outside of the current expression, and introduce a side-effect if we update the actual let binding's value.
To support the Right version of Project, we would need to traverse type-level constructors in the Dhall Expr type.
|
@JordanMartinez I noticed you added some more code after your last comment of "this is now ready for review", should I have a look or do you think there are more changes on the way? |
|
@f-f I feel like "the guy who cried wolf" but replacing 'wolf' with 'review.' 😅 I'd like to update the docs, and look over the branches again to verify that each is being tested in current tests. Then it'll be ready for a review. |
Previously, we were checking whether we should only update the `updateExpr` and only then falling back to updating either the `updateExpr` or the `recordExpr`. If the `updateExpr` was an exact match of the remaining fields we were seaching for, we would go down that path. Otherwise (i.e. the `updateExpr` was a partial match), we would try updating the `updateExpr` and then the `recordExpr` if the former failed By removing the "full" updateExpr case, we get the same execution but without the redundant code
While the immediate Var updates won't be as nice stylistically, they are always correct.
|
47a056e adds the test that checks this situation and fixes the result. I tried to make the update to the expression here prettier, but I think it could be an invalid update. So, I've decided to use an "always correct" modification that's not as pretty (though why one would be using let bindings here would be beyond me). -- always correct, but not pretty
let x = [ "console", "effect", "prelude", "psci-support" ] in x # [ "newtype" ]
-- pretty, but I think it can be incorrect?
let x = [ "console", "effect", "newtype", "prelude", "psci-support" ] in xSo the comments on the case branches have been updated. The remaining work to do here is:
|
|
@f-f This is ready for a review. I think with my "optimized via levelKeyStack" implementation for |
|
Apologies for the delayed response on this one - I have tried to convince myself that this should be merged ASAP, but I failed: the reason why I'm hesitant is that this PR adds a really big chunk of code (1.8k, of which 1k of Haskell, in a codebase of 6k lines, tests included) for implementing a feature that will need to change (we don't know how dramatically, but the fact that we don't know is part of the problem) once we ship the new Config format to support the Registry. I like this code and I'd like to have it in, but I would like to hold off until we ship the first version of the Registry, so we can consider this all together in the design of the new config. |
That's fine by me! I think your decision is wise. |
|
Superseded by #849 |
Description of the change
Fixes #813
Checklist:
READMEP.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊