core.typed

add support for user-defined dotted polymorphic function, and fix a bug

Details

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

Description

previously, user-defined function couldn't be annotate as dotted polymorphic function, this patch make this possible. Also, while writing the test case, I found that the code assume function's requried-params is more than or equal to the dom of function's type, but not less like `(fn [& y] (if (empty? y) nil (first y))) (All [x y ...] [x y ... y -> (U x nil)])`, so we may skip the function type `x`, I fixed this by changing the interface of check-fn-method1-rest-type from `(fn [rest drest kws]` to `(fn [remain-dom rest drest kws]`.

  1. polydot.diff
    23/Mar/14 12:08 AM
    8 kB
    Di Xu
  2. polydot-v2.diff
    24/Mar/14 6:43 AM
    8 kB
    Di Xu
  3. polydot-v3.diff
    24/Mar/14 10:58 AM
    7 kB
    Di Xu
  4. polydot-v4.diff
    24/Mar/14 9:31 PM
    7 kB
    Di Xu

Activity

Hide
Ambrose Bonnaire-Sergeant added a comment - - edited

Prefer `drop` over `nthrest` (it's lazy).

Instead of using a HVec (which in itself isn't accurate, the rest argument is a nilable non-empty seq), just use the same approach as previous, except take the union of the remaining dom and the dotted pretype.

Return something like:

(c/Un
  r/-nil
  (r/TApp-maker (r/Name-maker 'clojure.core.typed/NonEmptySeq)
  [(apply c/Un (or rest (.pre-type ^DottedPretype drest)) remain-dom)]))

Update the tests to reflect we're being a little less accurate here.

Great work!

Show
Ambrose Bonnaire-Sergeant added a comment - - edited Prefer `drop` over `nthrest` (it's lazy). Instead of using a HVec (which in itself isn't accurate, the rest argument is a nilable non-empty seq), just use the same approach as previous, except take the union of the remaining dom and the dotted pretype. Return something like:
(c/Un
  r/-nil
  (r/TApp-maker (r/Name-maker 'clojure.core.typed/NonEmptySeq)
  [(apply c/Un (or rest (.pre-type ^DottedPretype drest)) remain-dom)]))
Update the tests to reflect we're being a little less accurate here. Great work!
Hide
Ambrose Bonnaire-Sergeant added a comment -

gfredericks is having a related issue https://www.refheap.com/63657

Show
Ambrose Bonnaire-Sergeant added a comment - gfredericks is having a related issue https://www.refheap.com/63657
Hide
Di Xu added a comment -

I think

(apply c/Un (or rest (.pre-type ^DottedPretype drest)) remain-dom)

is not accurate enough, my patch generate type like

(U [x] [x y] [x y z] nil)

so that the `count` will filter out some type. I want to use `HeterogeneousSeq`, but it seems `first` don't recognize it.

Show
Di Xu added a comment - I think
(apply c/Un (or rest (.pre-type ^DottedPretype drest)) remain-dom)
is not accurate enough, my patch generate type like
(U [x] [x y] [x y z] nil)
so that the `count` will filter out some type. I want to use `HeterogeneousSeq`, but it seems `first` don't recognize it.
Hide
Ambrose Bonnaire-Sergeant added a comment -

I think your approach throws away the rest and drest information which is an issue.

HVec supports rest/drest, but HSeq does not. You can add support by copying how HeterogeneousVector does it.

Also we should introduce a HSequential that just extends clojure.lang.Sequential. Then we can use it to abstract over HVec, HSeq and HList, and use it in the first argument of `first`, rather than hard-coding HVec.

Which reminds me, clojure.lang.ISeq isn't sequential, so we need to ensure HSeq is sequential.

user=> (isa? clojure.lang.ASeq clojure.lang.Sequential)
true
user=> (isa? clojure.lang.ISeq clojure.lang.Sequential)
false

So we need to:

  • add support for rest/drest in HSeq
  • implement HSequential (with rest/drest support)
  • add cases in subtype/cs-gen for HSequential (on the right)
  • use HSequential in types like `first`
  • ensure we don't throw away rest/drest in check-fn-method1-rest-type
Show
Ambrose Bonnaire-Sergeant added a comment - I think your approach throws away the rest and drest information which is an issue. HVec supports rest/drest, but HSeq does not. You can add support by copying how HeterogeneousVector does it. Also we should introduce a HSequential that just extends clojure.lang.Sequential. Then we can use it to abstract over HVec, HSeq and HList, and use it in the first argument of `first`, rather than hard-coding HVec. Which reminds me, clojure.lang.ISeq isn't sequential, so we need to ensure HSeq is sequential. user=> (isa? clojure.lang.ASeq clojure.lang.Sequential) true user=> (isa? clojure.lang.ISeq clojure.lang.Sequential) false So we need to:
  • add support for rest/drest in HSeq
  • implement HSequential (with rest/drest support)
  • add cases in subtype/cs-gen for HSequential (on the right)
  • use HSequential in types like `first`
  • ensure we don't throw away rest/drest in check-fn-method1-rest-type
Hide
Di Xu added a comment -

well, updated patch according to your first comment, leave the improvement of accuracy for future

Show
Di Xu added a comment - well, updated patch according to your first comment, leave the improvement of accuracy for future
Hide
Ambrose Bonnaire-Sergeant added a comment -

Please pull the latest master, doesn't seem to patch cleanly.

A few things before I merge:

  • add ((some-fn nil? r/DottedPretype?) drest) to the FnResult contracts
  • I'm pretty sure the code you commented out is incorrect. It doesn't use the rest/drest type properly. better just to remove it and just leave a comment referencing this ticket. We can always look back at the ticket.
  • add (every? r/Type? remain-dom) to the :pre of check-fn-method1-rest-type
  • just pass (drop (count required-params) dom) as the first argument to check-fn-method1-rest-type. Otherwise we're traversing dom twice.
Show
Ambrose Bonnaire-Sergeant added a comment - Please pull the latest master, doesn't seem to patch cleanly. A few things before I merge:
  • add ((some-fn nil? r/DottedPretype?) drest) to the FnResult contracts
  • I'm pretty sure the code you commented out is incorrect. It doesn't use the rest/drest type properly. better just to remove it and just leave a comment referencing this ticket. We can always look back at the ticket.
  • add (every? r/Type? remain-dom) to the :pre of check-fn-method1-rest-type
  • just pass (drop (count required-params) dom) as the first argument to check-fn-method1-rest-type. Otherwise we're traversing dom twice.
Hide
Di Xu added a comment -

done, but assume you mean

((some-fn nil? (u/hvector-c? symbol? r/DottedPretype?)) drest)

by

((some-fn nil? r/DottedPretype?) drest)
Show
Di Xu added a comment - done, but assume you mean
((some-fn nil? (u/hvector-c? symbol? r/DottedPretype?)) drest)
by
((some-fn nil? r/DottedPretype?) drest)
Hide
Ambrose Bonnaire-Sergeant added a comment -

I actually meant on the RecurTarget contract, you've put the correct contract on FnResult.

Show
Ambrose Bonnaire-Sergeant added a comment - I actually meant on the RecurTarget contract, you've put the correct contract on FnResult.
Hide
Di Xu added a comment -

done

Show
Di Xu added a comment - done

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: