Allow wrapping constants so their location can be accessed

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.

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

The default behaviour is unchanged.

Result:

With wrapping enabled you'd get:

Result:

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.

Environment

None

Activity

Show:
Thomas Heller
March 18, 2017, 11:07 AM

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

Thomas Heller
March 18, 2017, 5:55 PM

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.

Thomas Heller
March 19, 2017, 8:35 AM

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.

import
May 10, 2017, 8:30 PM

Comment made by: aiba

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

Nicola Mometto
May 10, 2017, 9:46 PM

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.

Declined

Assignee

Nicola Mometto

Reporter

Thomas Heller

Labels

None

Approval

None

Patch

None

Priority

Major