tools.reader

Allow wrapping constants so their location can be accessed

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Declined
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

Currently the reader will lose all source information (line, column, end-line, end-column, file) on values that don't support metadata.

This change allows (optionally) wrapping everything that doesn't support metadata in the Constant record.

This would allow the CLJS analyzer to show correct locations that it currently cannot do in some circumstances.

(+ 1 "2")

The location of "2" is unknown and thus the location of the error is that of the +.

The default behaviour is unchanged.

(require '[clojure.tools.reader :as rdr])
(require '[clojure.tools.reader.reader-types :as rdrt])

(let [rdr (-> (rdrt/string-reader "[1 \"2\" nil true false 1.01])")
              (rdrt/indexing-push-back-reader))
      x (rdr/read {} rdr)]
  (binding [*print-meta* true]
    (prn x)))

Result:

^{:line 1, :column 1, :end-line 1, :end-column 28} [1 "2" nil true false 1.01]

With wrapping enabled you'd get:

(binding [rdr/*wrap-constants* true]
  (let [rdr (-> (rdrt/string-reader "[1 \"2\" nil true false 1.01])")
                (rdrt/indexing-push-back-reader))
        x (rdr/read {} rdr)]
    (binding [*print-meta* true]
      (prn x))))

Result:

^{:line 1, :column 1, :end-line 1, :end-column 28} [#clojure.tools.reader.Constant{:loc-info {:line 1, :column 2, :end-line 1, :end-column 3}, :value 1} #clojure.tools.reader.Constant{:loc-info {:line 1, :column 4, :end-line 1, :end-column 7}, :value "2"} #clojure.tools.reader.Constant{:loc-info {:line 1, :column 8, :end-line 1, :end-column 11}, :value nil} #clojure.tools.reader.Constant{:loc-info {:line 1, :column 12, :end-line 1, :end-column 16}, :value true} #clojure.tools.reader.Constant{:loc-info {:line 1, :column 17, :end-line 1, :end-column 22}, :value false} #clojure.tools.reader.Constant{:loc-info {:line 1, :column 23, :end-line 1, :end-column 27}, :value 1.01}]

The loc-info property of the Constant record could be moved to the metadata of the record.

The CLJS analyzer could easily unwrap this and extract the correct location info to merge that into the AST.

Activity

Hide
Thomas Heller added a comment -

The implementation is currently available here [1].

I first wanted to confirm that this is something worth considering before cutting actual patches. This change can easily be ported to the CLJS/EDN readers as well.

[1] https://github.com/clojure/tools.reader/compare/master...thheller:constant-wrapper

Show
Thomas Heller added a comment - The implementation is currently available here [1]. I first wanted to confirm that this is something worth considering before cutting actual patches. This change can easily be ported to the CLJS/EDN readers as well. [1] https://github.com/clojure/tools.reader/compare/master...thheller:constant-wrapper
Hide
Thomas Heller added a comment -

My initial enthusiasm was crushed by macroexpand. As soon as any macro does any kind of type detection on values it receives this whole approach breaks down. Unwrapping the form before handing it to macroexpand has the same effect as not having the information in the first place as analyze happens after expansion.

So while I still think this would be useful for tools, it is not viable for the analyzer.

Show
Thomas Heller added a comment - My initial enthusiasm was crushed by macroexpand. As soon as any macro does any kind of type detection on values it receives this whole approach breaks down. Unwrapping the form before handing it to macroexpand has the same effect as not having the information in the first place as analyze happens after expansion. So while I still think this would be useful for tools, it is not viable for the analyzer.
Hide
Thomas Heller added a comment -

Even from a tools perspective this isn't all that useful as it makes for sense to wrap everything and not eval tagged literals.

Sorry for the noise.

Show
Thomas Heller added a comment - Even from a tools perspective this isn't all that useful as it makes for sense to wrap everything and not eval tagged literals. Sorry for the noise.
Hide
Aaron Iba added a comment -

FWIW, I found this patch extremely useful when implementing a lein plugin to count lines of code [1]. I had to make further modifications, but this patch set me in the right direction. Without it, I might have given up.

If this functionality were to be incorporated in the mainstream tools.reader, I at least would find that useful.

[1] https://github.com/aiba/lein-count

Show
Aaron Iba added a comment - FWIW, I found this patch extremely useful when implementing a lein plugin to count lines of code [1]. I had to make further modifications, but this patch set me in the right direction. Without it, I might have given up. If this functionality were to be incorporated in the mainstream tools.reader, I at least would find that useful. [1] https://github.com/aiba/lein-count
Hide
Nicola Mometto added a comment -

[~alba] I appreciate the feedback, and understand that the functionality could be very useful for a certain domain of libs/plugins, but the patch as proposed is not general enoguh to be accepted and would prove to be quite invasive for certain applications (such as the clojurescript compiler) so I cannot accept this proposal. That said I'm definitely open for other proposals addressing this issue in a less problematic way.

Show
Nicola Mometto added a comment - [~alba] I appreciate the feedback, and understand that the functionality could be very useful for a certain domain of libs/plugins, but the patch as proposed is not general enoguh to be accepted and would prove to be quite invasive for certain applications (such as the clojurescript compiler) so I cannot accept this proposal. That said I'm definitely open for other proposals addressing this issue in a less problematic way.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: