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

consistent use of scForceOpen for generic dot field symbols #21738

Merged
merged 9 commits into from
May 5, 2023

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Apr 27, 2023

fixes #21724

@metagn metagn marked this pull request as draft April 27, 2023 13:47
@metagn

This comment was marked as outdated.

@metagn metagn changed the title always force open generic dot field symbols? force open generic standalone dot expression field symbols Apr 30, 2023
@metagn metagn changed the title force open generic standalone dot expression field symbols consistent scForceOpen for generic dot field symbols Apr 30, 2023
@metagn metagn marked this pull request as ready for review April 30, 2023 01:42
@metagn metagn changed the title consistent scForceOpen for generic dot field symbols consistent use of scForceOpen for generic dot field symbols Apr 30, 2023
@metagn

This comment was marked as outdated.

@metagn
Copy link
Collaborator Author

metagn commented May 3, 2023

This should be good to go. It's pretty straightforward, if we use scOpen and not scForceOpen and there is 1 overload with the dot field name, it locks it down to nkSym and calls markUsed. For dot fields however this is wrong as the dot field could be part of the object and not in scope (hence the bug). Previously this nkSym was detected after the fact and the symchoice was evaluated again and converted to nkClosedSymChoice, now it's just done once and handled correctly in the first place, evading the markUsed call as well.

The ambiguous strip test is to make sure the old behavior (converting to nkClosedSymChoice) stays consistent, as some packages had code that depended on it.

@@ -58,15 +58,22 @@ template isMixedIn(sym): bool =

proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
ctx: var GenericCtx; flags: TSemGenericFlags,
fromDotExpr=false): PNode =
fromDotExpr: static bool = false): PNode =
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have to be static bool? The code duplication is likely worse than a runtime check which is predicted correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't have to be, I thought since it's a small proc and called tons of times it might be better, but it's non-essential, changing it back

@Araq
Copy link
Member

Araq commented May 4, 2023

It breaks tests/js/tdiscard.nim ? Not sure.

@metagn
Copy link
Collaborator Author

metagn commented May 4, 2023

CI was fine before and it only broke on osx cpp, must have been bitflip/IO error

Chronos error should also be unrelated, maybe temporary server outage

Could rebase to be safe

@metagn
Copy link
Collaborator Author

metagn commented May 4, 2023

CI is fine now

@Araq Araq merged commit e92d768 into nim-lang:devel May 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2023

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

Hint: mm: orc; opt: speed; options: -d:release
166987 lines; 8.336s; 612.492MiB peakmem

capocasa pushed a commit to capocasa/Nim that referenced this pull request May 15, 2023
…#21738)

* always force open generic dot field symbols?

fixes nim-lang#21724 but might break code

* alternative, should fix CI

* other alternative, add test for previous CI failure

* not needed

* make sure call doesn't compile too

* ok actual second test

* ok final actual correct test

* apply performance idea

* don't make fromDotExpr static
capocasa pushed a commit to capocasa/Nim that referenced this pull request May 16, 2023
…#21738)

* always force open generic dot field symbols?

fixes nim-lang#21724 but might break code

* alternative, should fix CI

* other alternative, add test for previous CI failure

* not needed

* make sure call doesn't compile too

* ok actual second test

* ok final actual correct test

* apply performance idea

* don't make fromDotExpr static
narimiran pushed a commit that referenced this pull request Jun 15, 2023
* always force open generic dot field symbols?

fixes #21724 but might break code

* alternative, should fix CI

* other alternative, add test for previous CI failure

* not needed

* make sure call doesn't compile too

* ok actual second test

* ok final actual correct test

* apply performance idea

* don't make fromDotExpr static

(cherry picked from commit e92d768)
narimiran pushed a commit that referenced this pull request Jun 19, 2023
* always force open generic dot field symbols?

fixes #21724 but might break code

* alternative, should fix CI

* other alternative, add test for previous CI failure

* not needed

* make sure call doesn't compile too

* ok actual second test

* ok final actual correct test

* apply performance idea

* don't make fromDotExpr static

(cherry picked from commit e92d768)
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
…#21738)

* always force open generic dot field symbols?

fixes nim-lang#21724 but might break code

* alternative, should fix CI

* other alternative, add test for previous CI failure

* not needed

* make sure call doesn't compile too

* ok actual second test

* ok final actual correct test

* apply performance idea

* don't make fromDotExpr static
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler thinks an unused proc is used when an auto parameter is used
2 participants