Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

recent regression in generics as demo'd by jason package #16389

Closed
disruptek opened this issue Dec 18, 2020 · 15 comments
Closed

recent regression in generics as demo'd by jason package #16389

disruptek opened this issue Dec 18, 2020 · 15 comments

Comments

@disruptek
Copy link
Contributor

disruptek commented Dec 18, 2020

It passed CI on devel yesterday but today it doesn't. The error points to the typ = o.getTypeInst line:

Error: /home/runner/work/jason/jason/jason/jason.nim(313, 12) Error: generic instantiation too nested

... from https://github.com/disruptek/jason which has been rewritten even more simply since, but still demonstrates the error...

type   
  Json* = distinct string   ## Serialized JSON.

  JasonObject = concept j
    for v in fields(j):
      v is Jasonable

macro jason*(o: JasonObject): Json =
  ## Render an anonymous Nim tuple as a JSON array; objects and named
  ## tuples become JSON objects.

  let typ = o.getTypeInst
  if typ.kind != nnkTupleConstr:
    # use our object construction code for named tuples, objects
    result = jasonCurly(o)
  else:
    # use our array construction code for anonymous tuples
    result = jasonSquare(o)
@timotheecour
Copy link
Member

timotheecour commented Dec 18, 2020

can you try git bisect?

It passed CI on devel yesterday but today it doesn't

please show git hashes, avoids guess work

@disruptek
Copy link
Contributor Author

No guess work. Yesterday's nightly worked, today's nightly doesn't. The only significant PR I noticed was Araq's IC patch.

@timotheecour
Copy link
Member

timotheecour commented Dec 18, 2020

No guess work

imagine someone looking at this issue a week or a month from now, assuming this isn't fixed by that time; he'd have to match git log time with the time you submitted your issue and adjust time zone, etc. That's guess work. That's why issues need the github hash (it's even recommended in issue template), it avoids all guess work.

@disruptek
Copy link
Contributor Author

You're right; it'd be a shame to leave the regression unfixed for a month and have to do all that work.

@disruptek
Copy link
Contributor Author

Here's a link to the nightly release commit at the time of writing:
868c31e

@disruptek disruptek changed the title overnight regression in generics as demo'd by jason package recent regression in generics as demo'd by jason package Dec 19, 2020
@timotheecour
Copy link
Member

@disruptek what happens if you recompile nim with:
if c.instCounter > 50:
=>
if c.instCounter > largeValue:

this helps narrow down.
If justified we can do something analog to #13233 which added --maxLoopIterationsVM (or via intdefine)

I've also hit this in another other library.

@disruptek
Copy link
Contributor Author

I would rather not see another option added to the compiler. It would be better to fix the logic that makes the presumption that 50 is too many, especially since apparently the number of instantiations can change with the whims of compiler developers -- one day I used <50 and the next I used somewhere over 100. The constant was last changed in 2018.

@timotheecour
Copy link
Member

this helps narrow down.

my point was whether the regression you're observing generates an infinite number of instantiations or an increase in number of instantiations

@disruptek
Copy link
Contributor Author

So you think we should set an infiniteInsts flag before generating infinite instantiations and then check to see if it's set afterwards?

I don't understand the logic here.

The point I'm trying to make is that the constant shouldn't even exist. It's a technique @Araq likes to use, but that doesn't mean it's wise.

@timotheecour
Copy link
Member

That's not my point at all. To debug your issue, I'm asking whether the compiler now tries to generate just a larger nb of instantiations or whether it looks like it attempts to enter an infinite loop (eg as in a stack overflow).
Regarding constants: i hope you're not trying to solve the halting problem...

@Araq
Copy link
Member

Araq commented Dec 23, 2020

It's much wiser than runaway recursions in the compiler with its associated stack overflows (bad) or infinite loops (worse!)...

@disruptek
Copy link
Contributor Author

The irony is that, as regressions go, infinite loops don't live very long. Please fix the bug or revert the change. If the code doesn't work, I don't care if the compiler crashes -- it was never going to serve my needs in any event. There are cheaper ways (I'm looking at you, Gentoo) to heat my home.

@timotheecour
Copy link
Member

Please fix the bug or revert the change
Here's a link to the nightly release commit at the time of writing:

it's not clear from what you wrote whether this is the commit that introduced the regression or not 868c31e

@disruptek
Copy link
Contributor Author

I use your nightlies. The one prior to that was fine. The one I supplied was not.

@disruptek
Copy link
Contributor Author

I gave up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants