Skip to content

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Apr 19, 2025

fixes #5631, fixes #8938, fixes #18855, fixes #19271, fixes #23885, fixes #24877

isTupleRecursive, previously only called to give an error for illegal recursions for:

  • tuple fields
  • types declared in type sections
  • explicitly instantiated generic types

did not check for recursions in proc types. It now does, meaning proc types now need a nominal type layer to recurse over themselves. It is renamed to isRecursiveStructuralType to better reflect what it does, it is different from a recursive type that cannot exist due to a lack of pointer indirection which is possible for nominal types.

It is now also called to check the param/return types of procs, similar to how tuple field types are checked. Pointer indirection checks are not needed since procs are pointers.

I wondered if this would lead to a slowdown in the compiler but since it only skips structural types it shouldn't take too many iterations, not to mention only proc types are newly considered and aren't that common. But maybe something in the implementation could be inefficient, like the cycle detector using an IntSet.

Note: The name isRecursiveStructuralType is not exactly correct because it still checks for distinct types. If it didn't, then the compiler would accept this:

type
  A = distinct B
  B = ref A

But this breaks when attempting to write var x: A. However this is not the case for:

type
  A = object
    x: B
  B = ref A

So a better description would be "types that are structural on the backend".

A future step to deal with #14015 and #23224 might be to check the arguments of tyGenericInst as well but I don't know if this makes perfect sense.

@metagn metagn changed the title test disallowing proc recursions test disallowing proc type recursions Apr 19, 2025
@metagn metagn changed the title test disallowing proc type recursions disallow recursive structural proc types, check proc param types Apr 19, 2025
@metagn metagn changed the title disallow recursive structural proc types, check proc param types disallow recursive structural types, check proc param types Apr 19, 2025
@metagn metagn changed the title disallow recursive structural types, check proc param types generally disallow recursive structural types, check proc param types Apr 19, 2025
@metagn metagn marked this pull request as ready for review April 19, 2025 13:03
@Araq Araq merged commit 7f0e074 into nim-lang:devel Apr 21, 2025
18 checks passed
@Araq
Copy link
Member

Araq commented Apr 21, 2025

Definitely improves the situation but for the future we should remove the restriction that recursive types must go through a nominal type and instead simply accept these definitions and make sigmatch etc check for endless recursions.

Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 7f0e074

Hint: mm: orc; opt: speed; options: -d:release
179390 lines; 8.912s; 651.301MiB peakmem

metagn added a commit that referenced this pull request Apr 21, 2025
narimiran pushed a commit that referenced this pull request Apr 21, 2025
…#24893)

fixes #5631, fixes #8938, fixes #18855, fixes #19271, fixes #23885,
fixes #24877

`isTupleRecursive`, previously only called to give an error for illegal
recursions for:

* tuple fields
* types declared in type sections
* explicitly instantiated generic types

did not check for recursions in proc types. It now does, meaning proc
types now need a nominal type layer to recurse over themselves. It is
renamed to `isRecursiveStructuralType` to better reflect what it does,
it is different from a recursive type that cannot exist due to a lack of
pointer indirection which is possible for nominal types.

It is now also called to check the param/return types of procs, similar
to how tuple field types are checked. Pointer indirection checks are not
needed since procs are pointers.

I wondered if this would lead to a slowdown in the compiler but since it
only skips structural types it shouldn't take too many iterations, not
to mention only proc types are newly considered and aren't that common.
But maybe something in the implementation could be inefficient, like the
cycle detector using an IntSet.

Note: The name `isRecursiveStructuralType` is not exactly correct
because it still checks for `distinct` types. If it didn't, then the
compiler would accept this:

```nim
type
  A = distinct B
  B = ref A
```

But this breaks when attempting to write `var x: A`. However this is not
the case for:

```nim
type
  A = object
    x: B
  B = ref A
```

So a better description would be "types that are structural on the
backend".

A future step to deal with #14015 and #23224 might be to check the
arguments of `tyGenericInst` as well but I don't know if this makes
perfect sense.

(cherry picked from commit 7f0e074)
narimiran pushed a commit that referenced this pull request Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment