ClojureScript

Add ^string hints

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code

Description

There exist functions that are not automatically inferred as returning strings. Hinting these would help in the event that their return values are passed to numeric functions, or for code gen with CLJS-2865.

The set I believe needs explicit hints comprises:

subs
pr-str
prn-str
print-str
munge-str
clojure.string/reverse
clojure.string/replace
clojure.string/replace-first
clojure.string/join
clojure.string/upper-case
clojure.string/lower-case
clojure.string/capitalize
clojure.string/trim
clojure.string/triml
clojure.string/trimr
clojure.string/trim-newline
clojure.string/escape
  1. CLJS-2868-0.patch
    20/Feb/19 8:06 AM
    6 kB
    Martin Kučera
  2. CLJS-2868-1.patch
    19/Mar/19 8:39 AM
    7 kB
    Martin Kučera

Activity

Hide
Martin Kučera added a comment -

It's my first contribution, tell me if anything wrong!

Show
Martin Kučera added a comment - It's my first contribution, tell me if anything wrong!
Hide
Mike Fikes added a comment -

Confirmed with Martin Kučera via Slack that CA has been signed.

Show
Mike Fikes added a comment - Confirmed with Martin Kučera via Slack that CA has been signed.
Hide
Mike Fikes added a comment - - edited

Hey Martin, the way that subs is hinted in the patch leaves it as being inferred as any.

You can add a test in cljs.analyzer-tests like the following to see this.

(deftest test-cljs-2688
  (is (= (e/with-compiler-env test-cenv
           (:tag (analyze test-env '(subs "a" 0))))
        'string)))

With this in place you can check that it fails via

lein test :only cljs.analyzer-tests/test-cljs-2688

You can fix the issue by simply adding a single hint as in

(defn ^string subs
  "Returns the substring of s beginning at start inclusive, and ending
  at end (defaults to length of string), exclusive."
  ([s start] (.substring s start))
  ([s start end] (.substring s start end)))

I'd suggest adding is checks for each function of interest, and because of outward type hint propagation, you may find that some hints are unnecessary if the function's implementation involves something that can be inferred as being of type string. For example, a single hint on prn-str-with-opts might take care of pr-str, prn-str, or any other uses of prn-str-with-opts in the codebase.

Show
Mike Fikes added a comment - - edited Hey Martin, the way that subs is hinted in the patch leaves it as being inferred as any. You can add a test in cljs.analyzer-tests like the following to see this.
(deftest test-cljs-2688
  (is (= (e/with-compiler-env test-cenv
           (:tag (analyze test-env '(subs "a" 0))))
        'string)))
With this in place you can check that it fails via
lein test :only cljs.analyzer-tests/test-cljs-2688
You can fix the issue by simply adding a single hint as in
(defn ^string subs
  "Returns the substring of s beginning at start inclusive, and ending
  at end (defaults to length of string), exclusive."
  ([s start] (.substring s start))
  ([s start end] (.substring s start end)))
I'd suggest adding is checks for each function of interest, and because of outward type hint propagation, you may find that some hints are unnecessary if the function's implementation involves something that can be inferred as being of type string. For example, a single hint on prn-str-with-opts might take care of pr-str, prn-str, or any other uses of prn-str-with-opts in the codebase.
Hide
Martin Kučera added a comment -

Ok, I’ll create test for each function and then add type hints when necessary.

Show
Martin Kučera added a comment - Ok, I’ll create test for each function and then add type hints when necessary.
Hide
Mike Fikes added a comment -

Martin discovered that some of these cases involve variadic functions, and in those cases this ticket would ideally depend upon work in CLJS-2901 so that we can get away with fewer hints.

Show
Mike Fikes added a comment - Martin discovered that some of these cases involve variadic functions, and in those cases this ticket would ideally depend upon work in CLJS-2901 so that we can get away with fewer hints.
Hide
Mike Fikes added a comment -

Now that CLJS-2901 and CLJS-2865 are on master, it looks like these are now inferred as returning string:

pr-str
prn-str
print-str

leaving the others as simple hints to add.

Show
Mike Fikes added a comment - Now that CLJS-2901 and CLJS-2865 are on master, it looks like these are now inferred as returning string:
pr-str
prn-str
print-str
leaving the others as simple hints to add.
Hide
Mike Fikes added a comment -

It looks like, with master, which allows a different inferred hint on each arity, you can hint subs this way:

(defn subs
  "Returns the substring of s beginning at start inclusive, and ending
    at end (defaults to length of string), exclusive."
  ([s start] ^string (.substring s start))
  ([s start end] ^string (.substring s start end)))
Show
Mike Fikes added a comment - It looks like, with master, which allows a different inferred hint on each arity, you can hint subs this way:
(defn subs
  "Returns the substring of s beginning at start inclusive, and ending
    at end (defaults to length of string), exclusive."
  ([s start] ^string (.substring s start))
  ([s start end] ^string (.substring s start end)))
Hide
Mike Fikes added a comment - - edited

https://github.com/clojure/clojurescript/blob/f97d766defd02f7d43abd37e3e9b04790a521b1e/src/main/clojure/cljs/externs.clj#L165-L172 seems to imply there might be a way to ultimately access the type of expressions that bottom out in some Closure library functions

See CLJS-2987

Show
Mike Fikes added a comment - - edited https://github.com/clojure/clojurescript/blob/f97d766defd02f7d43abd37e3e9b04790a521b1e/src/main/clojure/cljs/externs.clj#L165-L172 seems to imply there might be a way to ultimately access the type of expressions that bottom out in some Closure library functions See CLJS-2987
Hide
Martin Kučera added a comment -

I tried to use the type hints from external libraries, but for the case of function built with JS method (e.g. cljs.core/subs use .substring on String) it doesn't seem like the right way because I must use explicit type hint for a target of JS method, otherwise I am not able to find the method in externals.

Code changes if I use type inferred from external libs:

(defn subs
  "Returns the substring of s beginning at start inclusive, and ending
    at end (defaults to length of string), exclusive."
  ([^string s start]  (.substring s start))
  ([^string s start end] (.substring s start end)))

That's why I think it's easy to use the original approach.

For infer type of dot JS method from external lib I suggest to create a separate task, because it doesn't add value for core function type infer and increase compile time.

Mike Fikes What do you think about it ?

Show
Martin Kučera added a comment - I tried to use the type hints from external libraries, but for the case of function built with JS method (e.g. cljs.core/subs use .substring on String) it doesn't seem like the right way because I must use explicit type hint for a target of JS method, otherwise I am not able to find the method in externals. Code changes if I use type inferred from external libs:
(defn subs
  "Returns the substring of s beginning at start inclusive, and ending
    at end (defaults to length of string), exclusive."
  ([^string s start]  (.substring s start))
  ([^string s start end] (.substring s start end)))
That's why I think it's easy to use the original approach. For infer type of dot JS method from external lib I suggest to create a separate task, because it doesn't add value for core function type infer and increase compile time. Mike Fikes What do you think about it ?
Hide
Mike Fikes added a comment - - edited

Yeah, I would only add ^string hints where we know they don't exist on Closure functions, and let a separate task handle inferring based on Closure metadata.

Show
Mike Fikes added a comment - - edited Yeah, I would only add ^string hints where we know they don't exist on Closure functions, and let a separate task handle inferring based on Closure metadata.
Hide
Martin Kučera added a comment -

Tags added.
Test of functions from core namespace was added.
Tests of functions from clojure.string namespace wasn't added because I don't know how to use it in the test.

Show
Martin Kučera added a comment - Tags added. Test of functions from core namespace was added. Tests of functions from clojure.string namespace wasn't added because I don't know how to use it in the test.
Hide
Mike Fikes added a comment -

Looks like all of these are good (revising the return type from any to string) except for clojure.string/join, which would need type hints on each arity instead (like subs).

Show
Mike Fikes added a comment - Looks like all of these are good (revising the return type from any to string) except for clojure.string/join, which would need type hints on each arity instead (like subs).
Hide
Martin Kučera added a comment -

Added typehint for string/join

Show
Martin Kučera added a comment - Added typehint for string/join
Hide
Martin Kučera added a comment -

Done

Show
Martin Kučera added a comment - Done

People

Vote (1)
Watch (1)

Dates

  • Created:
    Updated: