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

Remote imports #16

Merged
merged 29 commits into from
Sep 25, 2021
Merged

Remote imports #16

merged 29 commits into from
Sep 25, 2021

Conversation

mmhat
Copy link
Collaborator

@mmhat mmhat commented Sep 15, 2021

This PR adds the basic support for remote imports. That means:

  • The language is extended with a builtin URL literal:
    let Some/function = http://domain.tld/Some/function.ffg
    in Some/function "hello"
    
  • A type for resolvers newtype Resolver = Resolver { runResolver :: Text.URI.URI -> IO (Maybe Text) }.
    A resolver receives the URL as an argument and shall return Just code if can process the URL (e.g. an HTTP resolver can resolve http:// URLs but not file:// URLs.) and Nothing it not.
  • Several functions - like interpretWith or mainWith - take a value of type Grace.Interpret.InterpretSettings. Part of those settings is a list of resolvers which are tried one after another.
  • grace-core includes two simple resolvers:
    • A resolver for environment variables matching env:// URLs.
    • A resolver matching file:// URLs that is basically equivalent to the filepath-based imports.
    • A catchall resolver matching any url called External Resolver. This resolver tries to invoke a program grace-resolver-<scheme> ${URL}. If such an executable is not found in $PATH the resolver fails, i.e. does not match the URL passed to it.

@mmhat
Copy link
Collaborator Author

mmhat commented Sep 15, 2021

@Gabriel439 I think I am almost done with this - A little smoke testing, one more review of tests, code, documentation and so on; That's all what is left to do. Given that this topic involved a fair amount of design decisions I'd highly appreciate your comments on this PR 🙂

README.md Outdated Show resolved Hide resolved
grace-core/src/Grace/Interpret.hs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mmhat
Copy link
Collaborator Author

mmhat commented Sep 16, 2021

@Gabriel439 The last commits implement the ideas from #16 (comment). Please let me know if that is closer to the API you envision.

@mmhat
Copy link
Collaborator Author

mmhat commented Sep 17, 2021

Yet another approach for the configuration topic is here: https://github.com/mmhat/grace/tree/remote-imports-backpack
IMHO it is quite elegant but it has the downside that Stack users cannot use this project due to commercialhaskell/stack#2540.

The idea is as follows:

  • Keep the non-configurable part in grace-core.
  • The configurable interpreter lives in grace-interpreter. It requires the signature of Grace.Configuration to be instantiated when used.
  • grace-configuration-default provides the Grace.Configuration module for the default grace interpreter.
  • The grace package uses the former two to build the default interpreter.

TBH the main reason for this experiment is that I always wanted to play around with Backpack 😄

@Gabriella439
Copy link
Owner

Regarding the backpack solution, I still think it's a non-goal of this project to support package-level or API-level configurability, because I don't plan on publishing this project to Hackage. The intended user experience is that people will fork this project and edit it to implement features

The first reason why is to keep the implementation clearer for teaching purposes. The second reason why is because many desirable language features cannot (easily) be implemented by exposing a customizable interface.

As an example of the latter reason: in #17 you mentioned the introduction of a Map-like type (which I'm still thinking about), but suppose hypothetically that grace did not support such a built-in Map type and a downstream author wanted to add that type. It would be very difficult create an API that was sufficiently extensible to support such a type. For example, how would we make the Syntax tree or parser sufficiently customizable to store or parse such a type? It's possible to do, but very difficult.

The way I think about this project is that the source code is the API, not the actual Haskell API.

@mmhat
Copy link
Collaborator Author

mmhat commented Sep 17, 2021

@Gabriel439 Aaaah, okay! Thank you very much for clarification! 👍
I could have figured that from your comment above but I guess I was mentally stuck in the classic library development scenario... I apologize for the attention this PR draw from you.

Anyway, I will prepare this PR such that it does not have a configurable API and mark the respective parts as entrypoints for customization.

@mmhat
Copy link
Collaborator Author

mmhat commented Sep 17, 2021

@Gabriel439 Ok, this is now more in the spirit of this project: The interpreter does not take a callback function for URI resolving anymore and the grace-resolver-builtin package is gone.

grace-core/src/Grace/Import/Resolver.hs Outdated Show resolved Hide resolved
grace-core/src/Grace/Import/Resolver.hs Outdated Show resolved Hide resolved
grace-core/src/Grace/Import/Resolver.hs Outdated Show resolved Hide resolved
Also updated documentation of env resolver
grace-core/src/Grace/Import/Resolver.hs Outdated Show resolved Hide resolved
grace-core/src/Grace/Import/Resolver.hs Outdated Show resolved Hide resolved
grace-core/src/Grace/Interpret.hs Outdated Show resolved Hide resolved
grace-core/src/Grace/Interpret.hs Outdated Show resolved Hide resolved
grace-core/src/Grace/Lexer.hs Outdated Show resolved Hide resolved
grace-core/src/Grace/Parser.hs Outdated Show resolved Hide resolved
Copy link
Owner

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! Just a few last suggestions

grace-core/src/Grace/Import.hs Outdated Show resolved Hide resolved
tryAny (Import.resolverToCallback Resolver.defaultResolver uri)

(importExpression, directory') <- case eitherResult of
Left e -> throwError (ImportError uri (displayException e))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you will want to store the exception and not the rendered form of the exception, like this:

Suggested change
Left e -> throwError (ImportError uri (displayException e))
Left e -> throwError (ImportError uri e)

The main reason I suggest this is to preserve as much structure as possible from the original exception. Instead, render the underlying exception at the last moment (i.e. have the displayException method of ImportError call displayException for the wrapped exception).

The second reason that I suggest this is that you probably want the Show instance for ImportError to Show (and not display…) the underlying exception

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree with you; The reason I render the message already there is the Eq instance of ImportError. Sadly we have to choose between one of the two. What is your preference?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still doable if you use ExistentialQuantification, like this:

data InterpretError
    = forall e . (Exception e, Eq e) => ImportError URI e
    | …

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that we use tryAny to catch the exception, hence e :: SomeException which does not have an Eq instance. We could however change InterpretError to

data InterpretError
    = ImportError URI SomeException

and define the Eq instance like

instance Eq InterpretError where
    ImportError uri1 e1 == InterpretError uri2 e2 = uri1 == uri2 && show e1 == show e2

It is not a pretty "solution" but since the Eq instance is only intended to be used in the test suite it could work.

As an alternative we can add another constructor to Grace.Import.ImportError:

data ImportError
    = forall e. (Eq e, Exception e) => SomeImportError e
    | UnsupportedURI

Then we leave it to the resolvers to wrap their errors (and the one of the libraries they are using) and catch only this error in interpretExprWith.

grace-core/src/Grace/Interpret.hs Outdated Show resolved Hide resolved
mmhat and others added 5 commits September 22, 2021 03:20
* Consolidate `Import` and `Input` types into a single type for uniformity

* Fuse `parseInput` logic and resolver logic

* Consolidate `interpretWith`  and `interpretWithExpr`
@Gabriella439 Gabriella439 merged commit 320c184 into Gabriella439:main Sep 25, 2021
@Gabriella439
Copy link
Owner

I think this looks great! I think I can put up a PR of my own with the remaining proposed change for ImportError

@mmhat mmhat deleted the remote-imports branch September 25, 2021 19:03
@mmhat
Copy link
Collaborator Author

mmhat commented Sep 25, 2021

Thank you! 🚀

Regarding ImportError: Which path are you going to take?

@mmhat mmhat mentioned this pull request Oct 7, 2021
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