fixes #1027; disallow templates to use ambiguous identifiers #20631

ringabout · 2022-10-23T13:14:24Z

bung87 · 2022-10-24T13:16:57Z

doesn't this root cause is by ?

let candidates = searchInScopesFilterBy(c, ident, allExceptModule) #.skipAlias(n, c.config)
if candidates.len > 0:
  result = candidates[0]
  amb = candidates.len > 1
  if amb and checkAmbiguity in flags:
    errorUseQualifier(c, n.info, candidates)

after latest refactoring of searchInScopesFilterBy, candidates.len > 1 never will be true, which means checkAmbiguity become useless.

ringabout · 2022-10-24T13:21:06Z

after latest refactoring of searchInScopesFilterBy, candidates.len > 1 never will be true, which means checkAmbiguity become useless.

Where is it?

bung87 · 2022-10-24T13:25:39Z

after latest refactoring of searchInScopesFilterBy, candidates.len > 1 never will be true, which means checkAmbiguity become useless.

Where is it?

8f4bdb3

ringabout · 2022-10-24T13:27:28Z

8f4bdb3

I don't think it is related. That PR is regarding shadowing variables.

bung87 · 2022-10-24T13:32:44Z

8f4bdb3

I don't think it is related. That PR is regarding shadowing variables.

really? so how can candidates.len > 1 become true ?

ringabout · 2022-10-24T13:37:12Z

My PR 8f4bdb3 didn't change the correctness but the performance, two pieces of code are essentially identical.

bung87 · 2022-10-24T13:38:45Z

so maybe it doesn't work properly in previous ?

tests/template/m1027a.nim

tests/template/m1027b.nim

Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>

github-actions · 2022-10-24T19:49:22Z

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

Hint: mm: orc; opt: speed; options: -d:release
164033 lines; 7.958s; 613.152MiB peakmem

tersec · 2023-01-03T21:08:47Z

Would be useful to have this backported to 1.6 -- it silently generates invalid code.

arnetheduck · 2023-01-04T07:40:31Z

cc @narimiran

* test qualifiedLookUp in templates * check later * add testcase * add 4errormsg * Update tests/template/m1027a.nim Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com> * Update tests/template/m1027b.nim Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com> Co-authored-by: Andreas Rumpf <rumpf_a@web.de> Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>

* Add `nkFastAsgn` into `semExpr` (#20939) * Add nkFastAsgn into case statement * Add test case * fixes #1027; disallow templates to use ambiguous identifiers (#20631) * test qualifiedLookUp in templates * check later * add testcase * add 4errormsg * Update tests/template/m1027a.nim Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com> * Update tests/template/m1027b.nim Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com> Co-authored-by: Andreas Rumpf <rumpf_a@web.de> Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com> --------- Co-authored-by: Jake Leahy <jake@leahy.dev> Co-authored-by: Andreas Rumpf <rumpf_a@web.de> Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>

…im-lang#20631) * test qualifiedLookUp in templates * check later * add testcase * add 4errormsg * Update tests/template/m1027a.nim Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com> * Update tests/template/m1027b.nim Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com> Co-authored-by: Andreas Rumpf <rumpf_a@web.de> Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>

…le types in symchoices (#23997) fixes #23898, supersedes #23966 and #23990 Since #20631 ambiguous type symbols in templates are rejected outright, now we generate a symchoice for type nodes if they're ambiguous, a generalization of what was done in #22375. This is done for generics as well. Symchoices also handle type symbols better now, ensuring their type is a `typedesc` type; this probably isn't necessary for everything to work but it makes the logic more robust. Similar to #23989, we have to prepare for the fact that ambiguous type symbols behave differently than normal type symbols and either error normally or relegate to other routine symbols if the symbol is being called. Generating a symchoice emulates this behavior, `semExpr` will find the type symbol first, but since the symchoice has other symbols, it will count as an ambiguous type symbol. I know it seems spammy to carry around an ambiguity flag everywhere, but in the future when we have something like #23104 we could just always generate a symchoice, and the symchoice itself would carry the info of whether the first symbol was ambiguous. But this could harm compiler performance/memory use, it might be better to generate it only when we have to, which in the case of type symbols is only when they're ambiguous.

…le types in symchoices (#23997) fixes #23898, supersedes #23966 and #23990 Since #20631 ambiguous type symbols in templates are rejected outright, now we generate a symchoice for type nodes if they're ambiguous, a generalization of what was done in #22375. This is done for generics as well. Symchoices also handle type symbols better now, ensuring their type is a `typedesc` type; this probably isn't necessary for everything to work but it makes the logic more robust. Similar to #23989, we have to prepare for the fact that ambiguous type symbols behave differently than normal type symbols and either error normally or relegate to other routine symbols if the symbol is being called. Generating a symchoice emulates this behavior, `semExpr` will find the type symbol first, but since the symchoice has other symbols, it will count as an ambiguous type symbol. I know it seems spammy to carry around an ambiguity flag everywhere, but in the future when we have something like #23104 we could just always generate a symchoice, and the symchoice itself would carry the info of whether the first symbol was ambiguous. But this could harm compiler performance/memory use, it might be better to generate it only when we have to, which in the case of type symbols is only when they're ambiguous. (cherry picked from commit 09dcff7)

ringabout added 4 commits October 23, 2022 21:12

test qualifiedLookUp in templates

a32f8c0

check later

e26d3f6

add testcase

8e6fc1b

add 4errormsg

96ee25a

ringabout changed the title ~~test qualifiedLookUp in templates~~ fixes #1027; disallow templates to use ambiguous identifiers Oct 24, 2022

ringabout marked this pull request as ready for review October 24, 2022 05:26

Varriount reviewed Oct 24, 2022

View reviewed changes

tests/template/m1027a.nim Outdated Show resolved Hide resolved

Varriount reviewed Oct 24, 2022

View reviewed changes

tests/template/m1027b.nim Outdated Show resolved Hide resolved

Varriount requested a review from Araq October 24, 2022 16:59

Varriount added the Requires Araq To Merge PR should only be merged by Araq label Oct 24, 2022

Araq and others added 2 commits October 24, 2022 21:43

Update tests/template/m1027a.nim

4c6ef5d

Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>

Update tests/template/m1027b.nim

c2ca177

Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>

Araq merged commit 064b72a into devel Oct 24, 2022

Araq deleted the pr_qulify branch October 24, 2022 19:44

tersec mentioned this pull request Jan 3, 2023

eip4844 gossip status-im/nimbus-eth2#4444

Merged

metagn mentioned this pull request Aug 21, 2024

generate symchoice for ambiguous types in templates & generics + handle types in symchoices #23997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #1027; disallow templates to use ambiguous identifiers #20631

fixes #1027; disallow templates to use ambiguous identifiers #20631

ringabout commented Oct 23, 2022 •

edited

Loading

bung87 commented Oct 24, 2022 •

edited

Loading

ringabout commented Oct 24, 2022

bung87 commented Oct 24, 2022

ringabout commented Oct 24, 2022

bung87 commented Oct 24, 2022

ringabout commented Oct 24, 2022 •

edited

Loading

bung87 commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

tersec commented Jan 3, 2023

arnetheduck commented Jan 4, 2023

fixes #1027; disallow templates to use ambiguous identifiers #20631

fixes #1027; disallow templates to use ambiguous identifiers #20631

Conversation

ringabout commented Oct 23, 2022 • edited Loading

bung87 commented Oct 24, 2022 • edited Loading

ringabout commented Oct 24, 2022

bung87 commented Oct 24, 2022

ringabout commented Oct 24, 2022

bung87 commented Oct 24, 2022

ringabout commented Oct 24, 2022 • edited Loading

bung87 commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

tersec commented Jan 3, 2023

arnetheduck commented Jan 4, 2023

ringabout commented Oct 23, 2022 •

edited

Loading

bung87 commented Oct 24, 2022 •

edited

Loading

ringabout commented Oct 24, 2022 •

edited

Loading