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

Decouple responsibilities of Source class #185

Closed
pniederw opened this issue Jun 14, 2016 · 4 comments
Closed

Decouple responsibilities of Source class #185

pniederw opened this issue Jun 14, 2016 · 4 comments
Assignees
Labels

Comments

@pniederw
Copy link

pniederw commented Jun 14, 2016

Currently, Source has at least three distinct responsibilities:

  1. Load source code from different origins
  2. Cache source code once loaded
  3. Work with source text (SourceSection, etc.)

I'm happy with how Source handles 3, but have different requirements for 1 and 2 (i.e. I need full control over them). Can these responsibilities be extracted into separate classes (e.g. SourceLoader) that are not mandatory for using the PolyglotEngine/TruffleLanguage/Source APIs? Also, can you add a way to set the URI for a(ny) Source, so that its origin can be cleanly attached?

@jtulach
Copy link
Contributor

jtulach commented Jun 14, 2016

I am currently working on cleanup of Source creation sequence. I am going to introduce Source.Builder, draft is available at https://github.com/jtulach/truffle/tree/SourceBuilder

The builder will not use any caching - e.g. your point 2 will be satisfied.

I find the description of point 1 a little bit too vague (unless it means "set the URI for a Source") - hard to guess whether my work satisfies it.

@pniederw
Copy link
Author

To give some more background, I replaced 1 and 2 with my own code because:

  1. I wanted Source to behave uniformly regardless of which origin it was loaded from, in terms of eager vs. lazy loading, caching, etc.
  2. I wanted to support the following different kinds of source code origins specific to my language: URL, File, class path, standard library, REPL input, literal text
  3. I wanted to easily determine the origin of a piece of source code and query its origin-specific attributes (e.g. to perform permission checks and to resolve relative script includes in an origin-specific way), without having to reconstruct this information from a Source's answers to getURL, getPath, getName, etc. (which is brittle).
  4. I wanted to have my own implementations for loading code from an origin (e.g. to perform permission checks and to leverage non-blocking IO).
  5. I needed to have my own representation of source (origin) anyway, as leaking Truffle classes from my APIs wasn't an option.

To satisfy these requirements, I currently do the following: I do my own loading/caching, exclusively use Source.fromText to create Source objects, and keep origin-specific information about the source in an IdentityHashMap for later lookup. This works reasonably well. However, I'd rather not have to use IdentityHashMap, and complications arise when I need to process a Source that wasn't created by my own code. (I'd like to support this in order to be a good Truffle citizen.)

The following enhancements to Source could make it a more versatile abstraction and a better fit for my needs:

  1. Allow to attach opaque meta-information to a Source.
  2. Allow to easily determine which kind of built-in origin (fromURL, fromFilename, fromText, etc.) it was loaded from and what the origin-specific attributes are.

I also considered proposing a first-class, user-extensible notion of source origin, but this might not work so well in a polyglot setting.

@jtulach
Copy link
Contributor

jtulach commented Jun 15, 2016

After jtulach@872a1e9 the origin is taken uniformly, I'd say.

The additional meta-information is out of scope for the current "builder" change, but it is an interesting idea. Similar to: when a co-located source is about to be loaded (think of importScript('rel.js')) shouldn't that request be handled by the one who created the source that contains the importScript?

dougxc pushed a commit that referenced this issue Sep 6, 2016
…/truffle:sl_new_dsl_layout to master

* commit '2339890855bdafdccc589c7ae2fd9786308c5504':
  Add type system reference SLForeignToSLTypeNode to make it use the new layout.
  Use new DSL layout for SimpleLanguage.
  Deprecate DSLMetadata and remove it from the generator code.
@chumer chumer added the truffle label Jan 9, 2018
@chumer
Copy link
Member

chumer commented Apr 3, 2018

Looks like a stale issue where most issues are already resolved. Please reopen if addition work is necessary.

@chumer chumer closed this as completed Apr 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants