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

scope based symbol resolution; fixes generic sandwich problem; fix #13747, #17965, #13970: declarations at non-module scope now work with generics #18050

Closed

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented May 20, 2021

this PR fixes long standing major usability issues with generics

note

Example 1

# funs.nim:
proc bar*[T](a: T) =
  # mixin val
  doAssert val == 1, $val
let val* = 1

# main.nim:
import funs
# const val = 2
bar(0)

before PR

(eg: devel 1.5.1: 7f077a7)
nim r main passes
nim r main makes the val == 1 assert fail if you uncomment const val = 2
nim r main works again if you move const val = 2; bar(0) at block or proc scope.

after PR

  • all these give CT error Error: undeclared identifier: 'val'
  • if you uncomment mixin val, the CT errors go away
  • main.val (when const val = 2 is uncommented) is used regardless whether it's at module scope, block scope or proc scope

Example 2

(related to zevv/npeg#33 and mratsim/Arraymancer#508)
similar to Example 1 but with:

# stack.nim:
# does not define `NPegException`
proc grow*[T](s: var Stack[T]) =
  if s.top >= s.max:
    raise newException(NPegException, s.name & " stack overflow, depth>" & $s.max)
  s.frames.setLen s.frames.len * 2
# patt.nim:
import stack, common # common contains `NPegException`
grow(Stack[int].default)

before PR

  • compiles even though NPegException is not defined in stack.nim

after PR:

  • does not compile unless we add for eg mixin NPegException inside stack.grow

Example 3

(related to status-im/nim-stint#107)

# funs.nim:
template impl(T): untyped =
  when T is int: int
  else: float
type Foo*[T] = object
  f0*: impl(T)

# main.nim:
template impl(T): untyped = string # distractor
proc main[T](a: T)=
  var a: Foo[T]
  echo a.f0.type
main(1)
main(1.5)

before PR:

string
string

after PR:

int
float

Example 4

(related to #13747)

when true:
  proc fn[T](a: T): auto =
    mixin foo
    foo(a)
  proc main1 =
    proc foo(a: float): auto = "v1"
    doAssert fn(1.0) == "v1"
  main1()
  block:
    proc foo(a: int): auto = "v2"
    doAssert fn(1) == "v2"

before PR

CT error: Error: undeclared identifier: 'foo' even though it was mixed in

after PR

works

future work

  • address generic caching issue, which is a different topic, I have some ideas how to fix this in future work after this PR

CI failures

all the PR's i sent are forward and backward compatible

@timotheecour
Copy link
Member Author

status as of e425f0c: nim CI passes, 3 packages broken
(last commit: fixup, containing: # this is needed, see D20210519T200936 and D20210519T201000)

timotheecour added a commit to timotheecour/npeg that referenced this pull request May 21, 2021
@timotheecour timotheecour changed the title fix #13747, #17965, #13970: declarations at non-module scope now work with generics fix generic sandwich problem; fix #13747, #17965, #13970: declarations at non-module scope now work with generics May 21, 2021
timotheecour added a commit to timotheecour/Arraymancer that referenced this pull request May 22, 2021
timotheecour added a commit to timotheecour/zero-functional that referenced this pull request May 22, 2021
timotheecour added a commit to timotheecour/nim-stint that referenced this pull request May 23, 2021
timotheecour added a commit to timotheecour/pixie that referenced this pull request May 23, 2021
treeform added a commit to treeform/pixie that referenced this pull request May 23, 2021
@timotheecour timotheecour force-pushed the pr_fix_13747_generic_sandwich branch from 71ff07d to 8c4cf3f Compare May 25, 2021 21:56
zevv added a commit to zevv/npeg that referenced this pull request May 26, 2021
@Araq
Copy link
Member

Araq commented Jun 4, 2021

Very interesting solution but I don't really understand what it implies for the spec. Please write an RFC or at least patch the manual to reflect your approach.

@Clyybber
Copy link
Contributor

Clyybber commented Jun 9, 2021

This does not actually fix #11225 from what I can tell.

@timotheecour
Copy link
Member Author

timotheecour commented Jun 9, 2021

This does not actually fix #11225 from what I can tell.

#11225 was already fixed (by using bind and making bind work properly), it's a separate topic under the broader name of generic sandwich. I'm not claiming I'm fixing #11225 (and don't even reference it) in this PR.

@timotheecour timotheecour force-pushed the pr_fix_13747_generic_sandwich branch from 8c4cf3f to cf10455 Compare June 9, 2021 18:11
@@ -4,7 +4,23 @@ Utilities to help with debugging nim compiler.
Experimental API, subject to change.
]##

#[
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revert the changes to this file once the rest of the PR is accepted; I don't want to keep adding/reverting those locally (these changes make debugging easier)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, please explain your solution, somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's on my short-list TODO list

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friendly ping.

Copy link
Member Author

@timotheecour timotheecour Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok ok.

  • rebase
  • fix a couple TODO's
  • add explanation

EDIT: see timotheecour#763

@timotheecour timotheecour force-pushed the pr_fix_13747_generic_sandwich branch 2 times, most recently from d557746 to c365213 Compare June 22, 2021 00:54
@timotheecour timotheecour force-pushed the pr_fix_13747_generic_sandwich branch from c365213 to ad2c750 Compare June 22, 2021 01:33
@timotheecour timotheecour marked this pull request as ready for review June 22, 2021 04:20
@timotheecour timotheecour changed the title fix generic sandwich problem; fix #13747, #17965, #13970: declarations at non-module scope now work with generics scope based symbol resolution; fixes generic sandwich problem; fix #13747, #17965, #13970: declarations at non-module scope now work with generics Jun 22, 2021
mratsim pushed a commit to mratsim/Arraymancer that referenced this pull request Jun 30, 2021
@timotheecour timotheecour force-pushed the pr_fix_13747_generic_sandwich branch from ad2c750 to fea7ab0 Compare June 30, 2021 19:22
@timotheecour timotheecour force-pushed the pr_fix_13747_generic_sandwich branch from 02e906f to 68a42b6 Compare July 7, 2021 08:36
alehander92 added a commit to zero-functional/zero-functional that referenced this pull request Jul 7, 2021
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Jul 8, 2021
@timotheecour
Copy link
Member Author

timotheecour commented Jul 8, 2021

PTAL, all important_packages now pass (each package that needed patching involved a tiny patch, typically adding a mixin, and the patch made sense in each case)

@Araq
Copy link
Member

Araq commented Sep 19, 2021

PTAL, all important_packages now pass (each package that needed patching involved a tiny patch, typically adding a mixin, and the patch made sense in each case)

Sorry, I missed this remark. The change must first be opt-in regardless.

@stale
Copy link

stale bot commented Sep 20, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Sep 20, 2022
@stale stale bot closed this Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round stale Staled PR/issues; remove the label after fixing them
Projects
None yet
3 participants