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

Specification tests PR3 #59

Merged
merged 1 commit into from
Nov 28, 2021
Merged

Conversation

haxscramper
Copy link
Collaborator

No description provided.

@haxscramper
Copy link
Collaborator Author

I started using knownIssue feature already, so it should be implemented before this PR can pass CI and be merged

@haxscramper
Copy link
Collaborator Author

I added couple more knownIssues directly from the nim issue tracker - I don't plan to flood the test suite with everything I can find, but when it makes sense I will directly copy things.


impl((ref Derived)(nil))

block in_definition:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Related, although not fully identical issue - nim-lang/Nim#11136 it was closed because it "works as expected", but I'm not sure if this is a correct way to handle this.

This compiles, works (does not crash) and produces expected result? ((field: 12, afield: 1222)). Embedded part of the supertype is passed, modified and functions properly

type
  A = ref object of RootObj
    afield: int
    
  B = ref object of A
    field: int

proc init(a: var A) =
  a = A(afield: 1222)

var b = B(field: 123)
b.A.init() 
echo b[]

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 was decided that this particular example conforms to the spec because var ref Derived is not a subtype of Base. var Derived is, ref Derived and ptr Derived are subtypes as well, but double indirection makes it a non-subtype.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ref object being different from ref of the object is probably a bug though, otherwise there is a semantic difference between type Obj = ref object + arg: Obj and type Obj = object + arg: ref Obj, which does not makes sense to me, at least not now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That last one definitely doesn't make sense to me.

I think of it as doing the ref op on a type should return a ref type, don't really know how else it could work? Maybe it's to do with the silly auto dereference feature? 🤔


var derived = Derived(fderived: 128)

## It is possible to modify part of the embedded supertype via `var` argument
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe this should be moved to the "argument passing" part of the specification, but I'm not sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

argument passing styles/memory semantics makes sense, it'll be somewhat repetitive but that's not an issue.

Part of this shows which proc is called (belongs here), but the actual semantics of what's passed in and how mutations are allowed/do or don't show up is more about passing.

@haxscramper
Copy link
Collaborator Author

haxscramper commented Nov 19, 2021

I'm reading the issue tracker, here is a running to-do list that I will address as a part of this PR

@haxscramper
Copy link
Collaborator Author

Strict function and mutability analysis should also be covered now, since I started view types and path expressions seem to be tightly coupled with funcs.

@haxscramper
Copy link
Collaborator Author

haxscramper commented Nov 20, 2021

  • How static generic parameters work with overloading? Does it simply compare for equality? User-defined == in invoked in this case, or maybe there is something else that is used.
  • if both candidates have an equal number of generic argument matches (and no other matches), how best candidate is selected? See how it works for type classes, inline alternatives (|), exact types (including subtypes) or partially specified generics (including T; S: seq[T] case)

var y: set[char] = {}

# this one doesn't work (crashes the compiler)
var x: set[char] = (discard 12; {})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome, finding bugs!

@haxscramper
Copy link
Collaborator Author

haxscramper commented Nov 21, 2021

Vladar4/sdl2_nim#38 double bug - symbols leaking via imports (b does not export converter toInt) and failing overload resolution.

converter toInt*(x: uint8): int = int(x)

func toStr(x: int): string = "int"
func toStr(x: uint64): string = "uint64"

doAssert toStr(10'u8) == "uint64"

Is a correct code since integer widening has a higher precedence when overloading is considered, and presence of converter should not affect anything. uint64 is called when no converter is present, code with converter should behave the same way.

Moving converter into a separate file a -> b -> c should've fixed the code as well, since converter should not be invoked.

a.nim

converter toInt*(x: uint8): int =
  doAssert false
  int(x)

b.nim

import a

c.nim

import b

func toStr(x: int): string = "int"

doAssert toStr(10'u8) == "int"

This code should compile and execute without any runtime errors.

Manual is not clear enough on this part - uint8 is certainly within a int range (low(int) <= low(uint8) and high(uint8) <= high(int)) unless unsigned-to-signed conversion is disallowed.

Let f be the formal parameter's type and a the type of the argument.
Literal match: a is an integer literal of value v and f is a signed or unsigned integer type and v is in f's range.

func toStr(x: int): string = "int"

doAssert toStr(10'u8) == "int"

this code should compile and execute since f = int and v = 10'u8 and 10'u8 in low(int) .. high(int).

P.S. need to change the wording of manual as well, it is hard to reason about and requires keeping track of the f/v/a garbage. I mean, one-letter variables are good only if you don't care if anyone would read that ...


Looks like nim-lang/Nim#18350 is related


UPD: converter breaking the code is a regression in 1.6.0, but only when it comes to converter leaking. If I put code in a single file, it still fails, even though it does not make sense.

@haxscramper
Copy link
Collaborator Author

Maybe not in this PR, but nim-lang/Nim#18405 and other issues related to the typeof for iterators needs to be adressed.

var res: seq[
  typeof do:
    for word in ["a12"]:
      "a12"[0 .. 2]
]

Is a seq[char] while @["a12"[0 .. 2]] is a seq[string]. Also, I'm not sure how do I even put typeOfProc in the typeof do: call, everything I tried was a syntax error.

Comment on lines 190 to 196
block subtype_constaints_for_generic:
## When subtype is used as a constraint for the generic parameter it
## functions the same way as if it was written directly in the argument.

type
A = ref object of RootObj
B1 = ref object of A
B2 = ref object of A

proc impl[T](a: T): string = "T"
proc impl[T: A](a: T): string = "A"
proc impl[T: B1](a: T): string = "B1"
proc impl[T: B2](a: T): string = "B2"
proc impl[T: B1 | B2](a: T): string = "B1|B2"

doAssert impl(B1()) == "B1"
doAssert impl(B2()) == "B2"

## subtype vs generic - wins generic
doAssert impl(C()) == "matched generic"
## Note that `B1 | B2` is not reported as an ambiguous overload since constaints
## with alternatives have a lower precedence compared to the simple `SomeType`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this part make any sense wrt. to the implementation, or it is supposed to be an redefinition error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Expanded from nim-lang/Nim#6526

Copy link
Collaborator

Choose a reason for hiding this comment

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

That should be a redefinition error and the more specific and concrete the type is the more it should win.

The exception being that untyped and typed happen earlier and so they're special and should happen first.

@saem
Copy link
Collaborator

saem commented Nov 21, 2021

I think I have some guesses as to where the converter is leaking.

My concern is the struggle with precise description in some areas might be gaps in the spec, we're still developing a vocabulary to describe it.

@saem
Copy link
Collaborator

saem commented Nov 21, 2021

Maybe not in this PR, but nim-lang/Nim#18405 and other issues related to the typeof for iterators needs to be adressed.

var res: seq[
  typeof do:
    for word in ["a12"]:
      "a12"[0 .. 2]
]

Is a seq[char] while @["a12"[0 .. 2]] is a seq[string]. Also, I'm not sure how do I even put typeOfProc in the typeof do: call, everything I tried was a syntax error.

Yeah, I wouldn't handle it here, way too advance and some of these features we might end up killing or changing when we get to them.

@haxscramper
Copy link
Collaborator Author

haxscramper commented Nov 21, 2021

Reading issue tracker be like - "7 years old, unfixed", "same issue reported four years after". Random shouting match. Example, that takes one minute to minimize from 50+ loc to four. Something-something codegen error with basic misuse of the views which got "many new improvements for the 1.6.0". I don't know. I don't care. Link to PR that adds test that make my eyes bleed.

I managed to turn nim-lang/Nim#4799 (closed issue) into two new issues - varargs[var Base] results in failed codegen, and varargs[Base] where Base is a regular object downright crashes the compiler with

assign.nim(143)          genericAssign
assign.nim(129)          genericAssignAux
fatal.nim(53)            sysFatal
Error: unhandled exception: invalid object assignment [ObjectAssignmentDefect]


var derived = Derived(fderived: 128)

## It is possible to modify part of the embedded supertype via `var` argument
Copy link
Collaborator

Choose a reason for hiding this comment

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

argument passing styles/memory semantics makes sense, it'll be somewhat repetitive but that's not an issue.

Part of this shows which proc is called (belongs here), but the actual semantics of what's passed in and how mutations are allowed/do or don't show up is more about passing.

proc y(a: int or float; b: int or float) = discard
y(float(2.0), int(2))

proc x(a: Typeclass; b: Typeclass) = discard
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you have to add distinct to the type name in the proc definition, meaning the type parameters are distinct -- it's a different meaning.

like so:

proc x(a: Typeclass; b: distinct Typeclass) = discard

See the implicit generics section of the manual: https://nim-lang.org/docs/manual.html#generics-implicit-generics

I'm not sure how I feel about this though, I'm very much on the fence with a slight lean towards the current behaviour being correct with implicit generics.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that

proc impl(a: A or B, b: A or B) ...

and

type Alias = A or B
proc impl(a: Alias, b: Alias) ...

should function identically. If someone wanted to constrain both arguments to the specific "both A" or "both B" they could've written

type Alias = int or float
proc impl[T: Alias](a: T, b: T) = discard

impl(12.0, 2.0)

Copy link
Collaborator Author

@haxscramper haxscramper Nov 21, 2021

Choose a reason for hiding this comment

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

Intuitively, substituting A = <expr> in place of the <expr> should work the same way. A = <modifier> <expr> is different (like A = distinct <expr> etc.), but I don't think A = distinct int or float makes a lot of sense.

Name = distinct <concrete-type> is a new type (for the most part), Name = <concrete-type> is an alias, Name = <type1> | <type2> is a new typeclass. So Name = distinct <type1> | <type2> would also mean a new typeclass?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See the implicit generics section of the manual: nim-lang.org/docs/manual.html#generics-implicit-generics

I guess it makes sense, technically ... well, in that case I should probably consider that example works "according to the spec".

Found some interesting examples of the distinct typeclass feature

template `==`*(a, b: distinct (string|StringOfJson)): bool =

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'd have to play with the implemtnation a bit because here the identifier Alias is doing two things:

  1. referring to a specific type, in this case a typeclass, which makes the proc implicitly generic
  2. acting as the name T is in the second example you wrote

There are some subtleties that'll likely show up by working through the implementation, one example I can think of that's tricky is that in a generic proc you can refer to a type parameter like Alias or T in the body, so here is a case of an inner proc:

proc impl(a: Alias, b: Alias): Alias =
  proc identity(i: Alias): Alias = i
  identity(b)

let c = impl(1, "string")

Nim semantically analyses, due to metaprogramming/sanity reasons, in a forward direction, from leaf to root. So the compiler will:

  1. have a skeleton definition of the generic proc against the symbol impl
  2. then find the call to impl, it'll fix the two types for the generic instance
  3. in the generic instance, we either have an inner generic proc or do we have a fixed/known type?

So it's ambiguous, we can make a choice, but not sure we have enough information to choose which way is better.

Issues like the following come to mind with point 3:
a. Alias is only known if we treat it as a type parameter name, but
b. if it's also generic, then ok, I don't think that results in any codegen issues; but how do I say I don't mean generic
c. if we use impl[T: Alias] then if a is passed in a more precise sub-type of either int or string it'll fixate on that, and force b's type to narrow more than the Alias

although, now that I think about it, 'c' might be a bug already. 😬

Copy link
Collaborator Author

@haxscramper haxscramper Nov 21, 2021

Choose a reason for hiding this comment

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

I'm not sure how I feel about this though, I'm very much on the fence with a slight lean towards the current behaviour being correct with implicit generics.

I now think current behavior makes sense, so we should to leave it as is, and I will turn the example from "bug" into "test the error". I need to read more into distinct generics, they seem to be very weird at places, and not really widely used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

proc p(a: Table, b: distinct Table)

# is roughly the same as:

proc p[Key, Value, KeyB, ValueB](a: Table[Key, Value], b: Table[KeyB, ValueB])

Do you know what "roughly the same" means exactly in this case?

it means that the compiler sorta kinda does that transform, but significant details and all that differ.

Copy link
Collaborator

@saem saem Nov 21, 2021

Choose a reason for hiding this comment

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

I'm not sure how I feel about this though, I'm very much on the fence with a slight lean towards the current behaviour being correct with implicit generics.

I now think current behavior makes sense, so we need to leave it as is, and I will turn the example from "bug" into "test the error". I need to read more into distinct generics, they seem to be very weird at places, and not really widely used.

I've become less certain, here are the things I have more questions about, maybe @beef331 has thoughts.

Here are the various pieces I'm thinking of:

  • Alias the type parameter name
  • Alias, the name of a type or type alias in the type section
    • this is sorta ambiguous and very subtle, but is it the name of a type or an alias? current implementation leans alias
  • type or type value, in the type section Alias has typeclass2[int, string]
    • we could treat it as namedtypeclass2["Alias", int, string] (see alias vs type ambiguity above) but currently the implementation treats it like typeclass2[int, string]... mostly
  • type parameter value, the type value we bind to a given type parameter name, applying an unbound type parameter results in generics, while applying a bound type parameter resolves it and reduces the genericity

Some other misc thoughts, mostly settled:

  • pretty sure it isn't, but is distinct int or string ambiguous? is it distinct (int or string) or (distinct int) or string; precedence rules wise, distinct is kinda like a "type call" so that happens after the type expression int or string
  • distinct on a type parameter I believe has to apply to the type parameter name, so a type symbol operation, because if it applied to the type value it'd break the subtype relation

Things are still a bit off:

proc foo(a, b: int) = # `a` and `b` are both int
  discard

proc foo(a, b: int or string) = # `a` and `b` are which one of `int` or `string` don't have to be the same
  discard

type
  Base = object of RootObj
  Sub = object of Base

proc foo(a, b: Base) = # if I pass in a `Sub` for `a` it does _not_ fix `b`'s type
  discard

type Alias = int or string

proc foo(a, b: Alias) = # this issue

type Other = object of Base

proc foo(a, b: Base) = # also fine to pass in any combination of `Base, Sub, or Other`

proc foo(a, b: distinct Alias) = # works

proc foo(a, b: distinct int or string) = # doesn't work

My conclusion right now is that we stay close-ish to the manual, but the above is confusing, type alias vs type name, type parameter name and substitution, and type operators need to work more consistently.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's very counter intuitive, but it's also nice when you need it. The inheritance version probably should be bound identically, instead of just allowing any subtype, since an inheritable type is practically a constraint of anything that comes from it.

Should be noted you do not need a: T, b: distinct T the following works:

proc x(a, b: distinct SomeNumber) = echo typeof(a), " ", typeof(b)
x(10, 20)
x(10f, 20)
x(10u8, 20d)

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 is possible to pass multiple different subtypes to the varargs, and given mechanics of how the pass is performed (only embedded supertype part is actually passed) it makes sense that a, b: Base allows for Derived1(), Derived2()

@haxscramper
Copy link
Collaborator Author

type
  Base = object of RootObj
    fbase: int

  Derived = object of Base
    fderived: int

echo Derived().Base()
let z = Derived().Base()

echo works, let z fails

@haxscramper

This comment has been minimized.

@haxscramper
Copy link
Collaborator Author

    type
      CaveSeq[T] = object
        data: array[10, T]
        len: int

    proc add[T](s: var CaveSeq[T], data: T) =
      s.data[s.len] = data
      inc s.len

    var s: CaveSeq[Base]
    ceblock "Add base":
      s.add Base()

    ceblock "Add derived":
      s.add Derived()

    print s

works, but var s: seq[Base] fails, seq.add does not seem to follow regular rules for the argument passing

@haxscramper

This comment has been minimized.

@haxscramper

This comment has been minimized.

@haxscramper
Copy link
Collaborator Author

haxscramper commented Nov 22, 2021

All the bugs above were uncovered when I tried to specify how nim-lang/Nim#4799 should work exactly.

@haxscramper
Copy link
Collaborator Author

haxscramper commented Nov 23, 2021

  • add
type
  Vehicle[T] = object of RootObj
    tire: T
  Car[T] = object of Vehicle[T]
  Bike[T] = object of Vehicle[T]

proc testVehicle[T](x: varargs[Vehicle[T]]): string =
  result = ""
  for c in x:
    result.add $c.tire

var v = Vehicle[int](tire: 3)
var c = Car[int](tire: 4)
var b = Bike[int](tire: 2)
echo testVehicle([b, c, v])

Yes it should, moreover, it is not a test for varargs since you are first converting to openarray, and we only said that openarray/seq is compatible with varargs, not that ([a, b, c]) and (a, b, c) should behave identically. This might be the case, though, but it needs to be spelled out explicitly. You can also test passing with non-generic base and derived types with different parameters, as well as more nested inheritance hierarchy (NonGenBase -> Derived1[T], Derived2[T] and second derived also has two derived types). And test this all the (a, b, c), ([a, b, c]), (@[a, b, c]) cases if we say that openarray/seq passed to varargs should behave identically (which I think we can't say, because it makes sense to be able to pass varargs of var Base from the (a, b, c) but it is hardly possible to do with seq).

@haxscramper haxscramper marked this pull request as ready for review November 26, 2021 12:41
@haxscramper haxscramper mentioned this pull request Nov 26, 2021
5 tasks
@haxscramper
Copy link
Collaborator Author

haxscramper commented Nov 26, 2021

CI is green except for the "PR has only one commit" (what is the status on auto-squash again?). I decided that it would be better to wrap up tests I started rather than try to fully dig into surfaced issues, since in this case it could be a virtually endless process of finding more and more inconsistencies.

I decided to leave typeclass aliases for now as "expected behavior" in order to pinpoint opinion at least in one way.

Moved varargs tests to the t02_argument_passing_bugs.

Overloading and generics is not done yet, there are some things I completely omitted for now (implicit generics, some more bugs in the implementation), and things I'm yet to even spec out properly.

commit 2c58c4c
Author: haxscramper <[email protected]>
Date:   Fri Nov 26 19:19:22 2021 +0300

    [FIX] Final test cleanup

commit bee67ff
Merge: 350b2e2 14c784d
Author: haxscramper <[email protected]>
Date:   Fri Nov 26 19:01:10 2021 +0300

    Merge branch 'devel' into language-spec-3

commit 350b2e2
Author: haxscramper <[email protected]>
Date:   Fri Nov 26 19:00:18 2021 +0300

    [DOC] Overload resolution expected errors

commit 6936446
Author: haxscramper <[email protected]>
Date:   Fri Nov 26 16:48:57 2021 +0300

    [TEST] Vararags passing bugs

commit bf5959a
Merge: 507c910 e471f23
Author: haxscramper <[email protected]>
Date:   Sun Nov 21 22:35:56 2021 +0300

    Merge branch 'devel' into language-spec-3

commit 507c910
Author: haxscramper <[email protected]>
Date:   Sun Nov 21 22:28:11 2021 +0300

    [WIP] Varargs subtypes calls

commit 07fe117
Author: haxscramper <[email protected]>
Date:   Sun Nov 21 20:43:53 2021 +0300

    [WIP] Another bug of bugs

commit 0285a85
Author: haxscramper <[email protected]>
Date:   Sun Nov 21 17:02:50 2021 +0300

    [WIP] Generic constraints & subtypes

commit a0cbfbd
Author: haxscramper <[email protected]>
Date:   Fri Nov 19 23:19:55 2021 +0300

    [TEST] Overloading resolution

commit c04983a
Author: haxscramper <[email protected]>
Date:   Fri Nov 19 20:40:25 2021 +0300

    [TEST] Typeclass overload known issues

commit 4c60db7
Author: haxscramper <[email protected]>
Date:   Fri Nov 19 19:03:31 2021 +0300

    [TEST] View type borrow expressions

commit cbcff7c
Author: haxscramper <[email protected]>
Date:   Mon Nov 15 00:00:10 2021 +0300

    [WIP] Expand on templates

commit 805d122
Author: haxscramper <[email protected]>
Date:   Sun Nov 14 22:29:28 2021 +0300

    [DOC] Control flow with procedure calls

commit 4206347
Author: haxscramper <[email protected]>
Date:   Sun Nov 14 19:18:20 2021 +0300

    [DOC] Update toplevel spec readme

commit ac8d8dc
Author: haxscramper <[email protected]>
Date:   Sat Nov 13 18:06:21 2021 +0300

    [DOC] Specification tests PR3
@saem
Copy link
Collaborator

saem commented Nov 28, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 28, 2021

Build succeeded:

@bors bors bot merged commit 20e92d1 into nim-works:devel Nov 28, 2021
@haxscramper haxscramper changed the title [DOC] Specification tests PR3 Specification tests PR3 Dec 30, 2021
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