Another way to hang the compiler on recursive functions #1213
Labels
Milestone
Comments
|
That makes sense, it’s somewhat related to the problem outlined in pr #1173. Basically what happens is a call to meta before a binding is defined immediately evaluates the binding and fixes its type to a type variable. It also winds up creating extra bindings for a single form, so when we look things up we might get the wrong one (in this case we would have a local g in the function environment that is typed correctly, and a global g that is typed as a type var). If we make it so that sig actually assigns the designated type to the binding that’ll fix this. If we stop calling eval on meta-sets it’ll solve the double binding problem. Likely what’s happening here is that, during qualification, we somehow wind up finding the global g (created thanks to the meta set issue) instead of the local g, and then we lookup that symbol instead if the correctly typed local recursive one. Here are a few things to check related to fixing this throughly:
|
|
Great, all of that makes sense.
|
scolsen
added a commit
to scolsen/Carp
that referenced
this issue
May 24, 2021
This commit changes the behavior of expansions to avoid expanding module
expressions until we're actually processing the module in question.
Previously, the following form would be entirely expanded at the time of evaluating A:
```clojure
(defmodule A <- current environment
(some-macro) <- expand
(defmodule B
(some-macro f) <- expand, current env is A, *NOT* B.
so if this expands to
(private f)
(defn f ....)
the f of the expansion is added to *A*, and we have a duplicate
ghost binder.
)
(defn foo [] B.f) <- expand, B.f does not exist yet, any meta on the
binding will be ignored, permitting privacy errors since expansion
ignores undefined bindings, instead, we'll look this up at eval time,
and not check privacy as doing so would cause problems for legitimate
cases.
)
```
This meant that if the macro happened to have side-effects, e.g. calling
`meta-set!` we'd produce side-effects in the wrong environment, A,
resulting in duplicate bindings, missing bindings at evaluation time,
and other problems.
Now, we instead process the form as follows:
```clojure
(defmodule A <- current environment
(some-macro) <- expand
(defmodule B
(some-macro f) <- wait
)
(defn foo [] B.f)
)
;; step 2
(defmodule A
(foo-bar ) <- previously expanded macro
(defmodule B <- current environment
(some-macro f) <- expand
)
....
)
```
In general, this prevents the generation of a bunch of unintentional and
incorrectly added bindings when calling `meta-set!` from various macros.
Additionally, privacy constraints are now carried across nested modules:
```
(defmodule A
(defmodule B
(private f)
(defn f [] 0)
)
(defn g [] (B.f)) ;; Privacy error!
)
```
This change also fixed an issue whereby recursive functions with `sig`
annotations could trick the compiler. Again, this had to do with the
unintentionally added bindings stemming from expansion of nested module
expressions via meta-set.
Fixes carp-lang#1213, Fixes carp-lang#467
eriksvedang
pushed a commit
that referenced
this issue
May 24, 2021
* fix: don't expand inner module macros on first pass; privacy
This commit changes the behavior of expansions to avoid expanding module
expressions until we're actually processing the module in question.
Previously, the following form would be entirely expanded at the time of evaluating A:
```clojure
(defmodule A <- current environment
(some-macro) <- expand
(defmodule B
(some-macro f) <- expand, current env is A, *NOT* B.
so if this expands to
(private f)
(defn f ....)
the f of the expansion is added to *A*, and we have a duplicate
ghost binder.
)
(defn foo [] B.f) <- expand, B.f does not exist yet, any meta on the
binding will be ignored, permitting privacy errors since expansion
ignores undefined bindings, instead, we'll look this up at eval time,
and not check privacy as doing so would cause problems for legitimate
cases.
)
```
This meant that if the macro happened to have side-effects, e.g. calling
`meta-set!` we'd produce side-effects in the wrong environment, A,
resulting in duplicate bindings, missing bindings at evaluation time,
and other problems.
Now, we instead process the form as follows:
```clojure
(defmodule A <- current environment
(some-macro) <- expand
(defmodule B
(some-macro f) <- wait
)
(defn foo [] B.f)
)
;; step 2
(defmodule A
(foo-bar ) <- previously expanded macro
(defmodule B <- current environment
(some-macro f) <- expand
)
....
)
```
In general, this prevents the generation of a bunch of unintentional and
incorrectly added bindings when calling `meta-set!` from various macros.
Additionally, privacy constraints are now carried across nested modules:
```
(defmodule A
(defmodule B
(private f)
(defn f [] 0)
)
(defn g [] (B.f)) ;; Privacy error!
)
```
This change also fixed an issue whereby recursive functions with `sig`
annotations could trick the compiler. Again, this had to do with the
unintentionally added bindings stemming from expansion of nested module
expressions via meta-set.
Fixes #1213, Fixes #467
* fix: ensure we check privacy against the path of found binders
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I was experimenting with the fix for #349, and came up with this thing that still hangs:
Seems like the addition of the signature makes the difference, without it you get the appropriate warning.
The text was updated successfully, but these errors were encountered: