Skip to content

Conversation

@SirOlaf
Copy link
Contributor

@SirOlaf SirOlaf commented Jul 22, 2023

This PR aims to allow generic procs and templates to inherit generic parameters from their expected type, so either the lhs or a result expression.

Snippet adapted from tests, which contain other examples for how to use it

{.experimental: "inferGenericTypes".}

var x: seq[int]
x = newSeq(1)
doAssert x is seq[int]
doAssert x.len() == 1

Previously this would fail at compiletime with the error Error: cannot instantiate: 'T'

No existing code should break or be incompatible, this is a purely additive feature that in my opinion makes generics a bit nicer to use

@SirOlaf SirOlaf closed this Jul 22, 2023
@SirOlaf SirOlaf reopened this Jul 22, 2023
@SirOlaf SirOlaf marked this pull request as draft July 22, 2023 21:43
@SirOlaf
Copy link
Contributor Author

SirOlaf commented Jul 22, 2023

Seems to work surprisingly well, think this would be quite nice for things like Result[T] types where you can have an error that would otherwise require you to type it in some way (check tests for reference)

@SirOlaf SirOlaf changed the title Experiment: Infer generic bindings Infer generic bindings based on expected type Jul 22, 2023
@SirOlaf SirOlaf marked this pull request as ready for review July 22, 2023 23:10
@Araq
Copy link
Member

Araq commented Jul 23, 2023

This is superb stuff, thank you so much! But you need to document well how it works in the manual and what its restrictions are. Especially mention that this does not allow for overloading based on the return type (let x: T = f())

@SirOlaf
Copy link
Contributor Author

SirOlaf commented Jul 23, 2023

Thanks for the kind words, was honestly half expecting you to shut it down 😛

I assume this should go in a new section under generics?

@Araq
Copy link
Member

Araq commented Jul 23, 2023

I assume this should go in a new section under generics?

I suppose.

@SirOlaf
Copy link
Contributor Author

SirOlaf commented Jul 23, 2023

Done, if there's any more I should specify or generally things to change let me know

@Araq
Copy link
Member

Araq commented Jul 24, 2023

It would be super nice to have this for 2.0 but it needs to be behind an experimental flag for that. Also, watch your language in the manual, it should be more formal, there is no "you" and "the compiler", there is only how the type inference is performed, "who" does this is irrelevant.

@SirOlaf
Copy link
Contributor Author

SirOlaf commented Jul 24, 2023

Manual (hopefully) fixed, what should the name of this flag be?

Also does this mean it should go into the experimental manual instead?

@Araq
Copy link
Member

Araq commented Jul 24, 2023

experimental: inferGenericTypes

Also please add a test that ensures this works within object constructors where it comes up all the time too.

@SirOlaf
Copy link
Contributor Author

SirOlaf commented Jul 24, 2023

It does not currently work in constructors as that is done differently, but gonna look into it

@SirOlaf
Copy link
Contributor Author

SirOlaf commented Jul 24, 2023

Should it be allowed to infer only A) or also B)?
A is reasonably easy, not yet sure about B

type MyType[T] = object
  x : T

proc giveValue[T]: T = discard

# A)
let x = MyType[int](x : giveValue())

# B)
let y: MyType[int] = MyType(x : giveValue())

Edit: Case A works now, will look into B if desired

@SirOlaf
Copy link
Contributor Author

SirOlaf commented Jul 24, 2023

Gonna wait for tests and your opinion before continuing. Does this type reduction make sense or is it too expensive?
The tuple tests pass with it, so I'm thinking of using it for the entire inference later because it should be more accurate

@ASVIEST
Copy link
Contributor

ASVIEST commented Jul 25, 2023

Сurrent implementation does not support auto type.

proc giveValue[T]: auto = default(T)

let a: int = giveValue()

To fix it i propose to change

elif resNode.kind == nkIdent:

to

elif resNode.kind == nkIdent or resNode.typ != nil and resNode.typ.kind == tyAnything:

in inheritBindings proc

@SirOlaf
Copy link
Contributor Author

SirOlaf commented Jul 26, 2023

Thanks for pointing that out, completely forgot about auto. After looking at it though I don't really see a way to integrate auto into this smoothly for now because it acts weird when combined with "normal" generics.

Will be easier to see in a moment, am experimenting with that type reduction for the entire pass (seems broken though)

@SirOlaf SirOlaf marked this pull request as ready for review July 29, 2023 19:45
@SirOlaf
Copy link
Contributor Author

SirOlaf commented Jul 29, 2023

Now that this is ready for review once more, what changed since last time is that types now get reduced until only a flat generic parameter such as T is left on the lhs. Only after this is the type on the rhs bound to T.

This makes nesting much more consistent and allows tuples to participate as well.

Allowed the previous commit's CI run to finish with the flag not checked to verify that everything (that is tested) works as before.

@SirOlaf SirOlaf closed this Jul 29, 2023
@SirOlaf SirOlaf reopened this Jul 29, 2023
@SirOlaf SirOlaf closed this Jul 31, 2023
@SirOlaf SirOlaf reopened this Jul 31, 2023
@SirOlaf SirOlaf requested a review from Araq August 1, 2023 12:43
flatUnbound: seq[PType]
flatBound: seq[PType]
# seq[(result type, expected type)]
var typeStack = newSeq[(PType, PType)]()
Copy link
Member

Choose a reason for hiding this comment

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

What are the effects of this computation on the overall compile-times?

Copy link
Contributor Author

@SirOlaf SirOlaf Aug 2, 2023

Choose a reason for hiding this comment

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

Not sure, asked the same before without receiving a response. Will copy and paste a million lines or so to test (on that note it seems that hints/errors are all on the same line after line of uint16's max is reached)

Copy link
Contributor Author

@SirOlaf SirOlaf Aug 2, 2023

Choose a reason for hiding this comment

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

Using this code to generate two files, they both take about the same amount of time (immeasurable, they go back and forth. If you know of a better way to test please tell me). The cache was cleared before each compile. Memory usage in this specific test seems to be slightly better for the inferred version, but for other tests the one without inference uses less memory. The version of the nim compiler I used had the check for inferGenericTypes enabled.

Code
const copies = 10_000

block:
  var code = """{.experimental: "inferGenericTypes".}

type
  TestType[A, B, C, D, E, F, G, H, I, J] = object

proc giveData[A, B, C, D, E, F, G, H, I, J](): TestType[A, B, C, D, seq[E], F, G, H, I, seq[seq[J]]] = discard
var x: TestType[int, float, int, char, seq[byte], int16, uint32, seq[float], int, seq[seq[int]]] = giveData()
"""

  for i in 0 ..< copies:
    code.add("x = giveData()" & "\n")
  writeFile("out1.nim", code)

block:
  var code = """type
  TestType[A, B, C, D, E, F, G, H, I, J] = object

proc giveData[A, B, C, D, E, F, G, H, I, J](): TestType[A, B, C, D, seq[E], F, G, H, I, seq[seq[J]]] = discard
var x = giveData[int, float, int, char, seq[byte], int16, uint32, seq[float], int, seq[seq[int]]]()
"""

  for i in 0 ..< copies:
    code.add("x = giveData[int, float, int, char, seq[byte], int16, uint32, seq[float], int, seq[seq[int]]]()" & "\n")
  writeFile("out2.nim", code)

@SirOlaf SirOlaf changed the title Infer generic bindings based on expected type Add experimental inferGenericTypes switch Aug 2, 2023
@ASVIEST
Copy link
Contributor

ASVIEST commented Aug 2, 2023

I playing with your code and added auto type support (not full because it not retrieve generic parameter from out type in procedures) and simple case B) implementation

SirOlaf#1

@Araq Araq merged commit 8d8d757 into nim-lang:devel Aug 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

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

Hint: mm: orc; opt: speed; options: -d:release
168471 lines; 10.612s; 613.039MiB peakmem

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.

3 participants