tools.reader

Some column numbers off when reading symbols with source-logging-push-back-reader

Details

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

Description

I have not checked other variants of readers in tools.reader yet, but here are some steps to reproduce what I have found. Not big deals, just nice if the source code locations were not off by 1 in some cases. Search for "TBD" in the sample REPL session below to see the things that appear off to me.

(use '[clojure.tools.reader :only [read]])
(use 'clojure.pprint)
(require '[clojure.tools.reader.reader-types :as reader-types])

(defn slurp-already-opened [r]
  (let [sb (StringBuilder.)]
    (loop [c (reader-types/read-char r)]
      (if (nil? c)
        (str sb)
        (do
          (.append sb c)
          (recur (reader-types/read-char r)))))))

(defn read-with-locs [string]
  (let [reader (reader-types/source-logging-push-back-reader string)
        first-form (read reader)
        line (reader-types/get-line-number reader)
        column (reader-types/get-column-number reader)
        unread (slurp-already-opened reader)]
    {:form first-form
     :form-meta (meta first-form)
     :get-line line
     :get-column column
     :unread unread
     :read-length (- (count string) (count unread))}))

user=> (pprint (read-with-locs "foo"))
{:form foo,
 :form-meta
 {:line 1, :column 1, :end-line 1, :end-column 4, :source "foo"},
 :get-line 1,
 :get-column 3,  ; TBD: Expect 4 here
 :unread "",
 :read-length 3}
nil
user=> (pprint (read-with-locs "foo "))
{:form foo,
 :form-meta
 {:line 1, :column 1, :end-line 1, :end-column 5, :source "foo"},  ; TBD: Expect :end-column 4 here
 :get-line 1,
 :get-column 4,
 :unread " ",
 :read-length 3}
nil

Activity

Hide
Andy Fingerhut added a comment -

I don't have a proposed fix, but perhaps this is related to how most reader types do nothing for (unread nil) calls, but 2 of them, including source-logging-push-back-reader, do modify their state on such a call.

Show
Andy Fingerhut added a comment - I don't have a proposed fix, but perhaps this is related to how most reader types do nothing for (unread nil) calls, but 2 of them, including source-logging-push-back-reader, do modify their state on such a call.
Hide
Nicola Mometto added a comment -

I just pushed a potential fix for this on a branch, here's the relevant commit: https://github.com/clojure/tools.reader/commit/f32293fea5ac88895cb7909a3b034a60abd4031a

Among the other things, this patch makes column number 0-indexed as opposed to 1-indexed, representing the position between chars rather than the current char as that makes little sense in the context of newlines.

Can you try it out and let me know if you're still getting incongruent source infos? Thanks

Show
Nicola Mometto added a comment - I just pushed a potential fix for this on a branch, here's the relevant commit: https://github.com/clojure/tools.reader/commit/f32293fea5ac88895cb7909a3b034a60abd4031a Among the other things, this patch makes column number 0-indexed as opposed to 1-indexed, representing the position between chars rather than the current char as that makes little sense in the context of newlines. Can you try it out and let me know if you're still getting incongruent source infos? Thanks
Hide
Andy Fingerhut added a comment -

The 0-indexed change means it is incompatible with the built-in Clojure reader in column numbers? That seems like a change that would make it difficult to use as a drop-in replacement.

I will try out the changes and let you know, after perhaps hacking in a local change to add 1 to all column numbers.

Show
Andy Fingerhut added a comment - The 0-indexed change means it is incompatible with the built-in Clojure reader in column numbers? That seems like a change that would make it difficult to use as a drop-in replacement. I will try out the changes and let you know, after perhaps hacking in a local change to add 1 to all column numbers.
Hide
Nicola Mometto added a comment -

I hadn't considered LineNumberingPushbackReader compatibility – I pushed another commit to the col-changes branch reverting to 1-indexed column numbers. I'll probably make a mailing list post asking for feedback on this change as I'm of the opinion that 0-indexed column numbers make more sense, but this is admittedly an enhancement out of scope for this bug report.

Show
Nicola Mometto added a comment - I hadn't considered LineNumberingPushbackReader compatibility – I pushed another commit to the col-changes branch reverting to 1-indexed column numbers. I'll probably make a mailing list post asking for feedback on this change as I'm of the opinion that 0-indexed column numbers make more sense, but this is admittedly an enhancement out of scope for this bug report.
Hide
Andy Fingerhut added a comment - - edited

I tried out the changes with the 1-indexed column numbers, and in all of the test cases I have tried so far it seems to be doing what I expect, except perhaps not sure about comments with the source tracking reader. Here is an example, using the same read-with-locs function defined in the ticket description:

user=> (pprint (read-with-locs ";#1\nfoo "))
{:form foo,
 :form-meta
 {:line 2, :column 1, :end-line 2, :end-column 4, :source ";#1\nfoo"},
 :get-line 2,
 :get-column 4,
 :unread " ",
 :read-length 7}
nil

The line and column info is consistent with the location of the symbol 'foo'. The :source contains the preceding comment, and if there is more than one line containing a preceding comment, all of them, including intervening whitespace. That might be the only reasonable thing to do in this situation, but it seems worth mentioning, and perhaps documenting, if it is the expected behavior. Also adding a test case or 2 with comments, which I'd be happy to do if you like.

I have an updated file src/test/clojure/clojure/tools/metadata_test.clj with corrections to many :end-column values for the updated code, if that would be helpful.

Show
Andy Fingerhut added a comment - - edited I tried out the changes with the 1-indexed column numbers, and in all of the test cases I have tried so far it seems to be doing what I expect, except perhaps not sure about comments with the source tracking reader. Here is an example, using the same read-with-locs function defined in the ticket description:
user=> (pprint (read-with-locs ";#1\nfoo "))
{:form foo,
 :form-meta
 {:line 2, :column 1, :end-line 2, :end-column 4, :source ";#1\nfoo"},
 :get-line 2,
 :get-column 4,
 :unread " ",
 :read-length 7}
nil
The line and column info is consistent with the location of the symbol 'foo'. The :source contains the preceding comment, and if there is more than one line containing a preceding comment, all of them, including intervening whitespace. That might be the only reasonable thing to do in this situation, but it seems worth mentioning, and perhaps documenting, if it is the expected behavior. Also adding a test case or 2 with comments, which I'd be happy to do if you like. I have an updated file src/test/clojure/clojure/tools/metadata_test.clj with corrections to many :end-column values for the updated code, if that would be helpful.
Hide
Andy Fingerhut added a comment -

Attached patch trdr-19-update-metadata-tests-v1.patch updates metadata tests for recent changes to :end-column.

I have checked all of the changes by hand and they are what I expect them to be.

Show
Andy Fingerhut added a comment - Attached patch trdr-19-update-metadata-tests-v1.patch updates metadata tests for recent changes to :end-column. I have checked all of the changes by hand and they are what I expect them to be.
Hide
Nicola Mometto added a comment -

Thanks for the help, I just rebased my commits and pushed to master, including the patch you attached fixing the metadata test.
https://github.com/clojure/tools.reader/compare/52a95228cc...281b31a58c

The line/col info is orthogonal to the :source info of the source-logging readers and should be independent of comments/whitespaces so I'd say the current behaviour should be expected – agreed that some more documentation would be nice, I'll work on that.

Show
Nicola Mometto added a comment - Thanks for the help, I just rebased my commits and pushed to master, including the patch you attached fixing the metadata test. https://github.com/clojure/tools.reader/compare/52a95228cc...281b31a58c The line/col info is orthogonal to the :source info of the source-logging readers and should be independent of comments/whitespaces so I'd say the current behaviour should be expected – agreed that some more documentation would be nice, I'll work on that.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: