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

Fix #14844 #14863

Closed
wants to merge 15 commits into from
Closed

Fix #14844 #14863

wants to merge 15 commits into from

Conversation

Clyybber
Copy link
Contributor

One test will fail because the current broken behaviour allowed it to (https://play.nim-lang.org/#ix=2qAU).
Should both fail or work? A manual (var i = 0); (var i = 0) currently fails.. (for more context see bb532a6#diff-705c21e2cbb7aca092a9882547994008)

@timotheecour
Copy link
Member

Should both fail or work?

given that all these fail:

when defined case6:
  template bar*() =
    var r = @[(var i = @[""]; i), (var i = @[""]; i)]
    echo r
  bar()

when defined case7:
  var r = @[(var i = @[""]; i), (var i = @[""]; i)]
  echo r

when defined case8:
  proc main=
    var r = @[(var i = @[""]; i), (var i = @[""]; i)]
    echo r
  main()

it's better to be consistent; as well as to be consistent between MCS and regular call syntax, so the logical thing to do is make both fail; if absolutely needed add a -d:nimHasWorkaround14863 for a transition time.

Note that I'm fixing applyIt here to avoid the double evaluation bug: #14869

@Clyybber Clyybber force-pushed the dottedline branch 3 times, most recently from 10f82ce to a5f46f3 Compare July 24, 2020 11:48
@Clyybber Clyybber force-pushed the dottedline branch 6 times, most recently from 8fec9dc to bc23423 Compare October 13, 2020 13:06
@Clyybber Clyybber force-pushed the dottedline branch 2 times, most recently from 66a0088 to ca65f1c Compare October 31, 2020 14:35
@timotheecour
Copy link
Member

compiler/ast.nim Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Nov 2, 2020

This is very dangerous terrain.

It was testing that the behaviour is inconsistent
with what the code would look like written out manually :D
@Araq
Copy link
Member

Araq commented Mar 26, 2022

Too convoluted. Instead sem'checking should understand nkHiddenAddr, nkHiddenDeref and we need a nkHiddenArrayConstr node to make sem'checking idempotent.

@Araq Araq closed this Mar 26, 2022
@Clyybber
Copy link
Contributor Author

Clyybber commented Mar 26, 2022

That isn't related. This is about preventing typed AST ending up in untyped arguments.

@Araq
Copy link
Member

Araq commented Mar 27, 2022

And why is that a problem? "untyped" must go away, we need a solid typed AST so that macros can do type-based analysis. We don't need "I need untyped because the AST is then simpler", that's a symptom of the underlying issues.

@Araq Araq reopened this Mar 27, 2022
@stale
Copy link

stale bot commented Apr 2, 2023

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 Apr 2, 2023
@stale stale bot closed this May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants