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

Changing generic weight of tyGenericParam #22143

Merged
merged 58 commits into from
Jan 5, 2024

Conversation

Graveflo
Copy link
Contributor

This is in reference to a feature request that I posted.

I'm making this PR to demonstrate the suggested change and expect that this should be scrutinized

@Graveflo
Copy link
Contributor Author

On the CI failure, there is a test in the Nim codebase under tests/macros/tgetimpl that passes len as a typed parameter to a macro and then uses the expression y.owner where the parameter is y, as if len will not have other candidates. I'm not sure why this compiles with the current version in nim. It is given the nnkSym node kind in current nim and nnkClosedSymChoice in the PR'd version of nim. I'm still looking into the datamancer failure

Also, I know the test file is a little bit messed up. If preferable I can get rid of some of those "anti-regression" tests and I will fix the while true block inside of the block that I made.

Also, if this ends up working I think I should add a blurb to the manual

@Araq
Copy link
Member

Araq commented Jun 24, 2023

Also, if this ends up working I think I should add a blurb to the manual

You really should start with the manual/spec.

@Graveflo
Copy link
Contributor Author

Is there a formal spec? I looked but all I could find was the manual.
I'll add some text to the manual that talks about some of the pieces involved in this, as I think that clarification would be helpful in multiple ways.
To be clear, I don't think that I am trying to do make nim behave off-spec as it is described in the manual. It is a breaking change, but not contradicting the manual as far as I know. The test cases (barring the "anti regression" mess) are good isolated examples of what nim currently does vs what I assume it should do.
Also, I want to make sure I have my terminology correct here, because this should be important when explaining things in the manual. Is this accurate for the "anatomy of a call":

p[T: t](arg1: f): y

p: callee symbol

[...]: constraints

T: t: constraint

(arg1: f): y: formal signature

arg1: f: formal parameters

f: formal parameter type

y: formal return type

After matching / resolution the "formal" parameters that are not concrete crystalize into call-specific bindings.

@Araq
Copy link
Member

Araq commented Jun 24, 2023

Is there a formal spec? I looked but all I could find was the manual.

The manual is the spec. Could be more formal, we're getting there by telling contributors to write spec'y documentation.

@Araq
Copy link
Member

Araq commented Jun 24, 2023

It is:

p: callee symbol

[...]: generic parameters

T: t: generic constraint

[T: t](arg1: f): y: (formal) signature

arg1: f: (formal) parameter

f: (formal) parameter type

y: (formal) return type

@Graveflo
Copy link
Contributor Author

Alright, thank you and T is described as a "type variable" in the manual. This issue was a bit more complex then I realized.
I am going to:

  • Give the manual another look and see if there is more I can clarify there
  • Look at the datamancer failure in the CI but it is a web of macros that is hard to parse
  • Change the tgetimpl test to use something other then len because it has more then one candidate, so I think owner doesn't make sense there
  • Maybe make another issue with a condensed version of the bug in tgetimpl and make test cases for it, although I have no idea why it is currently accepted by the compiler
  • Look into other type classes in typeRel because I only modified how object behaves there and other type classes might need similar treatment

If I can iron out that list I might be done with this. For a first pass at least.

If anyone thinks that test in tgetimpl should actually work or has info about the datamancer failure feel free to let me know

@Graveflo

This comment was marked as outdated.

@Graveflo Graveflo marked this pull request as draft June 29, 2023 16:11
@Graveflo
Copy link
Contributor Author

PR #23033 appears to fix datamancer on this PR. As far as I know that is the last caveat here, so this should be ready if/when that gets merged

@Graveflo Graveflo marked this pull request as ready for review December 16, 2023 20:02
@Graveflo Graveflo marked this pull request as draft December 28, 2023 05:12
@Graveflo Graveflo marked this pull request as ready for review December 31, 2023 21:07
doc/manual.md Outdated Show resolved Hide resolved
doc/manual.md Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Jan 5, 2024

I'm merging this but it needs a follow PR as the documentation is incomprehensible. Also, the whole approach is kinda nuts (not your fault, of course). Instead we need a formal type relation isMoreSpecific(a, b) that describes the effects, not how it's implemented.

@Araq Araq merged commit 74fa8ed into nim-lang:devel Jan 5, 2024
12 of 19 checks passed
Copy link
Contributor

github-actions bot commented Jan 5, 2024

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

Hint: mm: orc; opt: speed; options: -d:release
177654 lines; 7.541s; 768.129MiB peakmem

@Graveflo
Copy link
Contributor Author

Graveflo commented Jan 6, 2024

we need a formal type relation isMoreSpecific(a, b) that describes the effects, not how it's implemented.

Just for posterity and to throw an idea out there: The hypothetical function isMoreSpecific maps to checkGeneric in the compiler source. Specificity is a hierarchical rank. typeRel defines a hierarchy implicitly and checkGeneric enforces it explicitly in overload resolution, so these two functions should be the same.

Documenting "specificity" would be 99% documenting typeRel which is an interesting prospect considering as how complex it is, and it's current documentation status. There's more that can be said about this but I wouldn't go into further detail with this prompting.

Also, for the record, I don't like the way specificity sounds in these contexts. I'm the one that started saying it, so not blaming anyone but me for that. Still, it does seem to fit the idea well enough.

#23171

narimiran pushed a commit that referenced this pull request Apr 18, 2024
The goal of this PR is to make `typeRel` accurate to it's definition for
generics:
```
# 3) When used with two type classes, it will check whether the types
# matching the first type class (aOrig) are a strict subset of the types matching
# the other (f). This allows us to compare the signatures of generic procs in
# order to give preferrence to the most specific one:
```

I don't want this PR to break any code, and I want to preserve all of
Nims current behaviors. I think that making this more accurate will help
serve as ground work for the future. It may not be possible to not break
anything but this is my attempt.

So that it is understood, this code was part of another PR (#22143) but
that problem statement only needed this change by extension. It's more
organized to split two problems into two PRs and this issue, being
non-breaking, should be a more immediate improvement.

---------

Co-authored-by: Andreas Rumpf <[email protected]>
(cherry picked from commit b2ca6be)
@Graveflo Graveflo deleted the generic-match-constraint branch July 24, 2024 22:34
narimiran pushed a commit that referenced this pull request Oct 4, 2024
This is in reference to a [feature
request](#22142) that I posted.

I'm making this PR to demonstrate the suggested change and expect that
this should be scrutinized

---------

Co-authored-by: Bung <[email protected]>
Co-authored-by: Andreas Rumpf <[email protected]>
(cherry picked from commit 74fa8ed)
narimiran pushed a commit that referenced this pull request Oct 4, 2024
This is in reference to a [feature
request](#22142) that I posted.

I'm making this PR to demonstrate the suggested change and expect that
this should be scrutinized

---------

Co-authored-by: Bung <[email protected]>
Co-authored-by: Andreas Rumpf <[email protected]>
(cherry picked from commit 74fa8ed)
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.

4 participants