-
-
Notifications
You must be signed in to change notification settings - Fork 181
imports: allow local imports for DHALL_HEADERS expression #1206
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
Conversation
The first step to standardizing this formally is to add a new boolean flag to the import resolution function that controls whether or not remote imports are enabled. Right now, the "type" of the import resolution function is:
… and you'd be changing it to add a boolean flag like this:
Then you will need to add and thread that flag through all of the import-resolution-related code Once you've done that then I'll show you how to update the |
OK, I think I've done the above. There were some cases I wasn't sure whether I should write Threading this flag through everything that touches imports seems kind of arduous, and I'm scared it might also cause us to fork the code (e.g. do we need to make Is there a way to restrict the additional logic to the headers resolution code? i.e. what if we always allow remote imports, but if you're silly enough to have a remote import in the headers expression itself then that import would be loaded with an empty "userheaders" context. I don't how how to put that in pseudocode, and I get the feeling you might be less comfortable with that because it's slightly closer to allowing infinite recursion, even though it still prevents it. |
Actually, I just had a thought. The import judgement already forbids cycles, doesn't it? What if we just lean on that? If we don't special-case the header resolution at all, wouldn't the existing rules cause it to fail on cyclic imports? As long as we include the relevant location ( Implementations would be free to provide a more useful error message in this specific case, but it wouldn't need to be standardised. |
@timbertson: Yeah, I like that idea even better. Just to make sure I understand: you're proposing that |
Yep, exactly. I'll update the PR later today
|
fac8547
to
5dbab24
Compare
5dbab24
to
a90e69f
Compare
standard/imports.md
Outdated
Γ(env:DHALL_HEADERS ? "${XDG_CONFIG_HOME}/dhall/headers.dhall" ? ~/.config/dhall/headers.dhall ? []) = userHeadersExpr | ||
(ε, here) × Γ₀ ⊢ userHeadersExpr ⇒ userHeaders ⊢ Γ₁ ; Resolve userHeadersExpr with an empty import context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might help clarify what here
is supposed to refer to
Γ(env:DHALL_HEADERS ? "${XDG_CONFIG_HOME}/dhall/headers.dhall" ? ~/.config/dhall/headers.dhall ? []) = userHeadersExpr | |
(ε, here) × Γ₀ ⊢ userHeadersExpr ⇒ userHeaders ⊢ Γ₁ ; Resolve userHeadersExpr with an empty import context, | |
headersPath = env:DHALL_HEADERS ? "${XDG_CONFIG_HOME}/dhall/headers.dhall" ? ~/.config/dhall/headers.dhall ? [] | |
Γ(headersPath) = userHeadersExpr | |
(ε, headersPath) × Γ₀ ⊢ userHeadersExpr ⇒ userHeaders ⊢ Γ₁ |
I've updated the pseudocode to resolve the expression, although I am discarding |
👋 any more feedback on this? Does it look reasonable to you @Gabriel439 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. This looks great! 🙂
As discussed in dhall-lang/dhall-haskell#2236, many authentication-related use cases of DHALL_HEADERS will rely on importing environment variables (e.g.
GITHUB_TOKEN
). So we want to allow local imports, but we don't want to allow remote imports (since that could get stuck in a loop).@Gabriel439 I've added prose and tests, but I might need your help to understand what pseudocode to update and how 🙏