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.

Sign up for GitHub

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

Jump to bottom

Fix(sigmatch): allow varargs[T] to accept overloaded symbol #19475

Closed
wants to merge 2 commits into from
Closed

Fix(sigmatch): allow varargs[T] to accept overloaded symbol #19475

wants to merge 2 commits into from

Conversation

ynfle
Copy link
Contributor

@ynfle ynfle commented Jan 31, 2022 •
edited
Loading

Fix(sigmatch): allow varargs[T] to accept overloaded symbol

There was a bug that params wouldn't be matched if the param was overloaded and the candidate type was varargs. This checks the base type of the varargs and then will to an overloaded symbol if in a macros.

I'm not sure what should be done in the case of a template.

There is a bug (which may be reported) of the following case:

import sequtils

template map(s: seq[int], p: typed): int =
  sequtils.map(s, p)

echo @[1, 2, 3, 4].map(len)

which sems to the "first" len but not he right one because it's a closed choice sym.

It is a separate issue, but it would now apply for a case of varargs also

Fixes #19446
Fixes #13913

@ynfle
Copy link
Contributor Author

ynfle commented Jan 31, 2022

The current devel is failing CI so not sure if this break anything. Make merging quite difficult

ynfle
ynfle commented Jan 31, 2022
View reviewed changes
compiler/sigmatch.nim Outdated
Comment on lines 2239 to 2242
if userConvMatch(c, m, f, a, arg) == nil and f.kind == tyVarargs:
if f.n == nil:
if typeRel(m, base(f), a) == isGeneric:
r = typeRel(m, base(f), a)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taken from

Nim/compiler/sigmatch.nim

Lines 2174 to 2182 in 33cd883

result = userConvMatch(c, m, f, a, arg)
# check for a base type match, which supports varargs[T] without []
# constructor in a call:
if result == nil and f.kind == tyVarargs:
if f.n != nil:
# Forward to the varargs converter
result = localConvMatch(c, m, f, a, arg)
else:
r = typeRel(m, base(f), a)

Could be more cases should be handled

@ringabout
Copy link
Member

Related: #19472

@ynfle ynfle force-pushed the fix-varargs-overload-match branch from d6e8554 to 9d31112 Compare January 31, 2022 12:35
@ynfle
Copy link
Contributor Author

ynfle commented Jan 31, 2022

I'm assuming this should be backported

@ynfle ynfle force-pushed the fix-varargs-overload-match branch from 9d31112 to abe0701 Compare January 31, 2022 14:23
Araq
Araq reviewed Feb 1, 2022
View reviewed changes
compiler/sigmatch.nim
@@ -2258,6 +2263,9 @@ proc paramTypesMatch*(m: var TCandidate, f, a: PType,
# See tsymchoice_for_expr as an example. 'f.kind == tyUntyped' should match
# anyway:
if f.kind in {tyUntyped, tyTyped}: result = arg
elif m.calleeSym != nil and m.calleeSym.kind in {skMacro, skTemplate} and
Copy link
Member

Choose a reason for hiding this comment

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

Likely caused by pre-existing code but the manual knows nothing about special casing overload resolution for macros and templates. So this is a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This basically copied from

Nim/compiler/sigmatch.nim

Line 2061 in 33cd883

m.calleeSym.kind in {skMacro, skTemplate}:

What are the semantics of a proc like len passed to a routine that isn't a macro or template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Araq

Copy link
Member

Choose a reason for hiding this comment

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

Well len is a tyProc. I don't understand the question I guess.

Araq
Araq reviewed Feb 1, 2022
View reviewed changes
compiler/sigmatch.nim Outdated Show resolved Hide resolved
@ynfle ynfle force-pushed the fix-varargs-overload-match branch from abe0701 to 0bc1c91 Compare February 3, 2022 23:13
ynfle
ynfle commented Feb 9, 2022
View reviewed changes
compiler/sigmatch.nim
if userConvMatch(c, m, f, a, arg) == nil and f.kind == tyVarargs:
if f.n == nil:
let baseRel = typeRel(m, base(f), a)
if baseRel == isGeneric:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should other cases be handled as well? When should it be a match but isn't isGeneric?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump

Copy link
Member

Choose a reason for hiding this comment

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

I don't know.

ynfle added 2 commits April 14, 2022 01:40
@ynfle
Fix(sigmatch): allow varargs[T] to accept overloaded symbol
7bfd754 
There was a bug that params wouldn't be matched if the param was
overloaded and the candidate type was varargs. This checks the base type
of the `varargs` and then will to an overloaded symbol if in a macros or
template.

Fixes nim-lang#19446 &
nim-lang#13913
@ynfle
Tests(sigmatch): Add tests for overloaded symbols for varargs[typed]
8b1bc9f 
This is only for macros as of now becasue I don't know what to do about
non macro routines
@ynfle ynfle force-pushed the fix-varargs-overload-match branch from d318fdb to 8b1bc9f Compare April 14, 2022 05:40
@ynfle
Copy link
Contributor Author

ynfle commented Apr 14, 2022

I don't think the failure is related to this PR.

@ringabout
Copy link
Member

I restarted the failed CI. The author can also restart CI without closing and reopening PRs => https://nim-lang.github.io/Nim/contributing.html#the-cmdgit-stuff-debugging-ci-failures-flaky-tests-etc

@ynfle
Copy link
Contributor Author

ynfle commented Apr 14, 2022

I restarted the failed CI. The author can also restart CI without closing and reopening PRs => https://nim-lang.github.io/Nim/contributing.html#the-cmdgit-stuff-debugging-ci-failures-flaky-tests-etc

Thanks

@stale
Copy link

stale bot commented Apr 25, 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 25, 2023
@github-actions
Copy link
Contributor

This pull request has been marked as stale and closed due to inactivity after 395 days.

@github-actions github-actions bot closed this Jun 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Araq left review comments

Assignees
No one assigned
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Milestone
No milestone
3 participants
@ynfle @ringabout @Araq