ClojureScript

Self-host: Allow :file key in cljs.js/*load-fn* callback

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: 1.7.145
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test

Description

Bootstrapped ClojureScript is abstracted away from direct I/O by use of a *load-fn* callback. A result is that when a namespace is loaded, the :file attribute associated with def s in [:cljs.analyzer/namespaces 'foo.ns :defs] in the AST is nil, because cljs.analyzer/*cljs-file* cannot be set to a meaningful value.

This ticket asks for an extension to *load-fn*, allowing a :file key to be optionally included by cljs.js clients, and for cljs.analyzer/*cljs-file* to be bound to that value in appropriate places in cljs.js so that the :file info appears in the AST.

One rationale for this :file attribute is that it makes it easier for clients of cljs.js to look up the file for a def, say, for use when implementing a source REPL special, for example.

  1. CLJS-1515-1.patch
    17/Dec/15 4:31 PM
    7 kB
    Andrea Richiardi
  2. CLJS-1515-2.patch
    18/Dec/15 5:29 PM
    17 kB
    Andrea Richiardi
  3. CLJS-1515-3.patch
    21/Dec/15 2:13 PM
    15 kB
    Andrea Richiardi
  4. CLJS-1515-4.patch
    21/Dec/15 9:48 PM
    69 kB
    Andrea Richiardi
  5. CLJS-1515-5.patch
    26/Dec/15 2:20 PM
    16 kB
    Andrea Richiardi
  6. CLJS-1515-6.patch
    14/Feb/16 9:17 PM
    17 kB
    Andrea Richiardi
  7. CLJS-1515-7.patch
    10/Aug/16 7:07 PM
    14 kB
    Andrea Richiardi

Activity

Hide
Andrea Richiardi added a comment -

Initial patch, adding a :file key to load-fn and a :file-env key inside opts and then assigning it to cljs.analyzer/cljs-file in eval-str. This approach can be discussed and we can create an ad-hoc function for binding. It felt right there.
Moreover, cljs.analyzer/cljs-file gets overridden every time with the payload coming from load-fn.
All this was very quickly done in order to have a feedback from who's more expert than me about the consequences. This is also my very first ClojureScript patch

Show
Andrea Richiardi added a comment - Initial patch, adding a :file key to load-fn and a :file-env key inside opts and then assigning it to cljs.analyzer/cljs-file in eval-str. This approach can be discussed and we can create an ad-hoc function for binding. It felt right there. Moreover, cljs.analyzer/cljs-file gets overridden every time with the payload coming from load-fn. All this was very quickly done in order to have a feedback from who's more expert than me about the consequences. This is also my very first ClojureScript patch
Hide
Mike Fikes added a comment -

I tried this patch. It is working fine for me when loading namespaces, but if I use cljs.js/analyze-str where the string is an ns form referring other namespaces loaded via *load-fn*, along with a def, things are off. (I have that ns referring macros from a clj file and a symbol from a cljs file, and the clj file gets associated with the top-level def and the macro, and the def in the referred file ends up with nil.

As a minor aside, the patch has a spurious whitespace change at the end.

Show
Mike Fikes added a comment - I tried this patch. It is working fine for me when loading namespaces, but if I use cljs.js/analyze-str where the string is an ns form referring other namespaces loaded via *load-fn*, along with a def, things are off. (I have that ns referring macros from a clj file and a symbol from a cljs file, and the clj file gets associated with the top-level def and the macro, and the def in the referred file ends up with nil. As a minor aside, the patch has a spurious whitespace change at the end.
Hide
Mike Fikes added a comment -

With respect to the last comment: The patch employs the pattern of conveying the :file passed in the cb via a :file-env opt to the consuming fn. It is consumed in eval-str* but not in analyze-str*. If the same logic is added to analyze-str* then the problem mentioned in the last comment goes away.

Show
Mike Fikes added a comment - With respect to the last comment: The patch employs the pattern of conveying the :file passed in the cb via a :file-env opt to the consuming fn. It is consumed in eval-str* but not in analyze-str*. If the same logic is added to analyze-str* then the problem mentioned in the last comment goes away.
Hide
David Miller added a comment -

I'm hopeful someone will assign this to a responsible party. I am not that person.

Show
David Miller added a comment - I'm hopeful someone will assign this to a responsible party. I am not that person.
Hide
Andrea Richiardi added a comment -

sorry David (Miller) and thanks Mike, I will rework it, adding some tests as well

Show
Andrea Richiardi added a comment - sorry David (Miller) and thanks Mike, I will rework it, adding some tests as well
Hide
Andrea Richiardi added a comment - - edited

By the way this makes me think that maybe a better choice is to consider this a side effect and directly modify cljs.analyzer/*cljs-file* returning from *load-fn*, who knows how many other spots I am not covering...

Show
Andrea Richiardi added a comment - - edited By the way this makes me think that maybe a better choice is to consider this a side effect and directly modify cljs.analyzer/*cljs-file* returning from *load-fn*, who knows how many other spots I am not covering...
Hide
Mike Fikes added a comment -

Two more comments:

1) Broadening the scope of the binding doesn't appear to work properly for me. But things do work if the bindings are done as in the patch now (next to where the other bindings are done).

2) Perhaps :file should be only set if the :lang being called back with is :clj. Maybe this could at least be documented. (It is not clear to me if it is useful for :js, as the patch is setting ana/*cljs-file*.)

Show
Mike Fikes added a comment - Two more comments: 1) Broadening the scope of the binding doesn't appear to work properly for me. But things do work if the bindings are done as in the patch now (next to where the other bindings are done). 2) Perhaps :file should be only set if the :lang being called back with is :clj. Maybe this could at least be documented. (It is not clear to me if it is useful for :js, as the patch is setting ana/*cljs-file*.)
Hide
Andrea Richiardi added a comment -

About 2), is any AST generated for .js files at all? If yes maybe then we should add it too...I need to explore that code path as well.

Show
Andrea Richiardi added a comment - About 2), is any AST generated for .js files at all? If yes maybe then we should add it too...I need to explore that code path as well.
Hide
Andrea Richiardi added a comment - - edited

So basically with ana/*cljs-file* binding the :file in :meta is not changed at all (I fixed following Mike's advice) but :file is, are we ok with this? In replumb (from planck) we check both so no problem, nonetheless it would be great to know why..

:defs {foo {:protocol-inline nil, :meta {:file bootstrap-test.core, :line 3, :column 7, :end-line 3, :end-column 10, :arglists (quote ([a b]))}, :name bootstrap-test.core/foo, :variadic false, :file /.../clojurescript/src/test/self/bootstrap_test/core.cljs, :end-column 10, :method-params ([a b]), :protocol-impl nil, :arglists-meta (nil nil), :column 1, :line 3, :end-line 3, :max-fixed-arity 2, :fn-var true, :arglists (quote ([a b]))}}, :require-macros nil, :doc nil

Show
Andrea Richiardi added a comment - - edited So basically with ana/*cljs-file* binding the :file in :meta is not changed at all (I fixed following Mike's advice) but :file is, are we ok with this? In replumb (from planck) we check both so no problem, nonetheless it would be great to know why.. :defs {foo {:protocol-inline nil, :meta {:file bootstrap-test.core, :line 3, :column 7, :end-line 3, :end-column 10, :arglists (quote ([a b]))}, :name bootstrap-test.core/foo, :variadic false, :file /.../clojurescript/src/test/self/bootstrap_test/core.cljs, :end-column 10, :method-params ([a b]), :protocol-impl nil, :arglists-meta (nil nil), :column 1, :line 3, :end-line 3, :max-fixed-arity 2, :fn-var true, :arglists (quote ([a b]))}}, :require-macros nil, :doc nil
Hide
Andrea Richiardi added a comment -

It looks like the information in :meta comes directly from the multimethod parse which I dont' think we can change easily. So either we override :file in :meta or we leave as it is with a note in the documentation for :file in *load-fn*

Show
Andrea Richiardi added a comment - It looks like the information in :meta comes directly from the multimethod parse which I dont' think we can change easily. So either we override :file in :meta or we leave as it is with a note in the documentation for :file in *load-fn*
Hide
Andrea Richiardi added a comment - - edited

About :js files at least to me it looks like the only trace of importing, say, goog.sting in the AST is in the :imports of the parent namespace. No :file key anywhere, but please correct me if I am wrong as the AST is difficult to untangle

Show
Andrea Richiardi added a comment - - edited About :js files at least to me it looks like the only trace of importing, say, goog.sting in the AST is in the :imports of the parent namespace. No :file key anywhere, but please correct me if I am wrong as the AST is difficult to untangle
Hide
Andrea Richiardi added a comment -

Patch and test

Show
Andrea Richiardi added a comment - Patch and test
Hide
Mike Fikes added a comment -

Comments on {{CLJS-1515-2.patch}} (mostly just opinion):

  1. (Opinion): Introduces new public API, especially with respect to AST exposure. Perhaps fn could instead be added to the test namespace.
  2. (Opinion): I wouldn't try anything complicated to try to patch up the :file that is in the :meta map. (Maybe we'll ultimately figure out why setting cljs.analyzer/*cljs-file* is insufficient for that bit.)
  3. (Opinion): For the :file docstring, I'd avoid mentioning AST. (Even though that was the true motivation for this ticket.) I'd only indicate that it represents the location where :source was obtained. (Which I guess would leave open it being perfectly fine for clients to provide it in the case that :lang is :js.)
  4. script/test-self-host passes for me.
  5. Inadvertent whitespace changes in append-source-map.
Show
Mike Fikes added a comment - Comments on {{CLJS-1515-2.patch}} (mostly just opinion):
  1. (Opinion): Introduces new public API, especially with respect to AST exposure. Perhaps fn could instead be added to the test namespace.
  2. (Opinion): I wouldn't try anything complicated to try to patch up the :file that is in the :meta map. (Maybe we'll ultimately figure out why setting cljs.analyzer/*cljs-file* is insufficient for that bit.)
  3. (Opinion): For the :file docstring, I'd avoid mentioning AST. (Even though that was the true motivation for this ticket.) I'd only indicate that it represents the location where :source was obtained. (Which I guess would leave open it being perfectly fine for clients to provide it in the case that :lang is :js.)
  4. script/test-self-host passes for me.
  5. Inadvertent whitespace changes in append-source-map.
Hide
Andrea Richiardi added a comment -

1. Sorry Mike I don't understand when you say fn...what do you mean? Can you expand?
2. Yes and it would change a lot of code, that's why I didn't even try
3. Ok can change that, but where should be mentioned that we are modifying :file but not inside :meta?
4. Great!
5. You know I really tried hard not to have that, I will try again to disable all the auto indent my emacs has.

Show
Andrea Richiardi added a comment - 1. Sorry Mike I don't understand when you say fn...what do you mean? Can you expand? 2. Yes and it would change a lot of code, that's why I didn't even try 3. Ok can change that, but where should be mentioned that we are modifying :file but not inside :meta? 4. Great! 5. You know I really tried hard not to have that, I will try again to disable all the auto indent my emacs has.
Hide
Mike Fikes added a comment -

1. The three new public functions in cljs.js: (var-ast, ns-ast, file->lang) could perhaps be moved to be utility functions in the self-host test namespace.
3. Dunno about the :meta question. But on the :lang :js question, perhaps the patch should only bind :cljs.analyzer/*cljs-file* if :lang :clj?

Show
Mike Fikes added a comment - 1. The three new public functions in cljs.js: (var-ast, ns-ast, file->lang) could perhaps be moved to be utility functions in the self-host test namespace. 3. Dunno about the :meta question. But on the :lang :js question, perhaps the patch should only bind :cljs.analyzer/*cljs-file* if :lang :clj?
Hide
Andrea Richiardi added a comment -

1. I know it looks like they are used in test only, but I put them there as public because both replumb and planck use them and I was kind of "proposing" this kind of AST utils to be part of the official API (so that the poor dev does not have to go through cljs.analyzer in order to query the AST. I understand if no though.
3. This I don't really know, and seek guidance. I have not noticed any significant change in the AST for .js file, maybe *cljs-file* is never queried in that code path. I could not even find a way to test it. But I could, of course, be very wrong.

Show
Andrea Richiardi added a comment - 1. I know it looks like they are used in test only, but I put them there as public because both replumb and planck use them and I was kind of "proposing" this kind of AST utils to be part of the official API (so that the poor dev does not have to go through cljs.analyzer in order to query the AST. I understand if no though. 3. This I don't really know, and seek guidance. I have not noticed any significant change in the AST for .js file, maybe *cljs-file* is never queried in that code path. I could not even find a way to test it. But I could, of course, be very wrong.
Hide
Andrea Richiardi added a comment -

This puts the utils functions in the test namespace for now, maybe thinking about exposing some API in the future.

Show
Andrea Richiardi added a comment - This puts the utils functions in the test namespace for now, maybe thinking about exposing some API in the future.
Hide
Andrea Richiardi added a comment -

About :js:

  • it looks like the analyze-str code path simply recurs to fetch the next dep. So I guess that branch does not touch the AST.
  • for the require code path it looks like it -> is -> similar.

Therefore I don't see the point in adding :file for :js and I will not bind *cljs-file* if this is the case, as you suggested.

Show
Andrea Richiardi added a comment - About :js:
  • it looks like the analyze-str code path simply recurs to fetch the next dep. So I guess that branch does not touch the AST.
  • for the require code path it looks like it -> is -> similar.
Therefore I don't see the point in adding :file for :js and I will not bind *cljs-file* if this is the case, as you suggested.
Hide
Andrea Richiardi added a comment - - edited

Patch #4 changes the conveying key to :cljs-file, after Mike's good suggestion, and moves the assoc to the (condp ... :clj) branch only. I also added a test to check that *cljs-file* does not match the file path when in the :js branch.

Show
Andrea Richiardi added a comment - - edited Patch #4 changes the conveying key to :cljs-file, after Mike's good suggestion, and moves the assoc to the (condp ... :clj) branch only. I also added a test to check that *cljs-file* does not match the file path when in the :js branch.
Hide
Andrea Richiardi added a comment - - edited

Another note, the *cljs-file* test works because the binding form does not actually restore the old value when it exits...In Clojure it would not probably work.

^ This is plain wrong, I was not considering the "when" my tests are executed, please disregard.

Show
Andrea Richiardi added a comment - - edited Another note, the *cljs-file* test works because the binding form does not actually restore the old value when it exits...In Clojure it would not probably work. ^ This is plain wrong, I was not considering the "when" my tests are executed, please disregard.
Hide
Mike Fikes added a comment -

CLJS-1515-4.patch LGTM.

Details: I tested against current ClojureScript master, using downstream Planck to load regular and macro namespaces and the :file portion of the AST gets properly updated. This also occurs if I instead use cljs.js/analyze-str passing in an ns form that causes code to be loaded. Additionally unit tests (regular and bootstrap) pass for me. I think this patch is functionally good to go.

Show
Mike Fikes added a comment - CLJS-1515-4.patch LGTM. Details: I tested against current ClojureScript master, using downstream Planck to load regular and macro namespaces and the :file portion of the AST gets properly updated. This also occurs if I instead use cljs.js/analyze-str passing in an ns form that causes code to be loaded. Additionally unit tests (regular and bootstrap) pass for me. I think this patch is functionally good to go.
Hide
David Nolen added a comment -

Copying goog.string into the source tree is not desirable. Please fix the tests to remove this. If you must, copy it to a temporary a location from the Google Closure Library JAR and remove it after the test has completed, thanks.

Show
David Nolen added a comment - Copying goog.string into the source tree is not desirable. Please fix the tests to remove this. If you must, copy it to a temporary a location from the Google Closure Library JAR and remove it after the test has completed, thanks.
Hide
Andrea Richiardi added a comment - - edited

Patch 5 avoid copying string.js and re-uses self_host/test.js.

Show
Andrea Richiardi added a comment - - edited Patch 5 avoid copying string.js and re-uses self_host/test.js.
Hide
Andrea Richiardi added a comment -

Done what you asked

Show
Andrea Richiardi added a comment - Done what you asked
Hide
Mike Fikes added a comment -

CLJS-1515-5.patch no longer applies

Show
Mike Fikes added a comment - CLJS-1515-5.patch no longer applies
Hide
Andrea Richiardi added a comment -

Reapplied and re-tested. Works

Testing with Node

Testing self-host.test

Ran 8 tests containing 47 assertions.
0 failures, 0 errors.
Show
Andrea Richiardi added a comment - Reapplied and re-tested. Works
Testing with Node

Testing self-host.test

Ran 8 tests containing 47 assertions.
0 failures, 0 errors.
Hide
Andrea Richiardi added a comment -

All self-test pass!

Show
Andrea Richiardi added a comment - All self-test pass!

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: