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

Type error: wrong type match #188

Closed
Namek opened this issue Feb 16, 2020 · 5 comments
Closed

Type error: wrong type match #188

Namek opened this issue Feb 16, 2020 · 5 comments
Labels
question Further information is requested

Comments

@Namek
Copy link
Contributor

Namek commented Feb 16, 2020

Issue

When assigning a value the compiler seems to guess a type of value and then it type-checks the value with a state field. This results in an error where properly defined data structure cannot be compiled.

In the code below both islands and connections look the same:

state puzzle : Puzzle = {
  islands =
    {
      list = [],
      fields = Map.empty()
    },
  connections =
    {
      list = [],
      fields = Map.empty()
    },
  width = 0,
  height = 0,
  maxConnectionCount = 2
}

However, internally they are different in all (2) subfields:

record Connections {
  list : Array(Connection),
  fields : Map(Number, IndexPair)
}

record Islands {
  list : Array(Island),
  fields : Map(Number, Bool)
}

Expected behaviour

Since I've defined my state puzzle to be a specific type (here Puzzle) I would expect that to be a priority. Thus, type-check should continue the search.

Reproduction

https://sandbox.mint-lang.com/sandboxes/tweRxXyq9aa8Rg

@gdotdesign
Copy link
Member

This is because the array literal is ambiguous: [] basically means Array(a)

In this specific case the type signature of the state would allow us to match the type but it would not solve the bigger issue stated above.

I would suggest to do what Crystal does: specify the type of the empty array as [] of Type

In that case this would not be ambiguous:

state puzzle : Puzzle = {
  islands =
    {
      list = [] of Islands,
      fields = Map.empty()
    },
  connections =
    {
      list = [] of Connections,
      fields = Map.empty()
    },
  width = 0,
  height = 0,
  maxConnectionCount = 2
}

Of course this would be a big breaking change 🚧

@gdotdesign gdotdesign added the question Further information is requested label Feb 17, 2020
@teggotic
Copy link

@gdotdesign Why not just force to use types of parent record type?

@gdotdesign
Copy link
Member

gdotdesign commented Feb 17, 2020

Why not just force to use types of parent record type?

The answer for this is that we don't know the parent record the beforehand.


To explain what is happening here: basically the compiler "sees" records in isolation (without type definitions) like this:

{
  islands =
    {
      list = [],
      fields = Map.empty()
    },
  connections =
    {
      list = [],
      fields = Map.empty()
    },
  width = 0,
  height = 0,
  maxConnectionCount = 2
}

and it's type is inferred to be this:

{
  islands :
    {
      list : Array(a),
      fields : Map(a, b)
    },
  connections :
    {
      list : Array(a),
      fields : Map(a, b)
    },
  maxConnectionCount : Number,
  height : Number,
  width : Number,
}

and after finding a type Islands for this:

{
  list : Array(a),
  fields : Map(a, b)
}

it becomes this:

{
  islands : Islands
  connections : Islands,
  maxConnectionCount : Number,
  height : Number,
  width : Number,
}

Which does not match any defined type.


An other way to go would be to add type definition to records as {...} as Puzzle then we would match the record definition to the actual record itself.

This is one of the reasons why Crystal has the as construct (https://crystal-lang.org/reference/syntax_and_semantics/as.html).

@Namek
Copy link
Contributor Author

Namek commented Feb 17, 2020

Thank you for the explanation.

I (think I) understand the algorithm but from a user experience it's very tedious to play with type system and guess what's bad. If I'm made (only at this point, hopefully) to explicitly define variable/state types then I want to do it for something. It's not nice to provide the typename twice:

state puzzle : Puzzle = { ... } as Puzzle

I know this is not easy but I believe this really needs an improvement. And since we would aim for better error messages then this could be a good step to improve the type-matching algorithm.

The algorithm would be:

  1. look for constraints (like, type of variable) types and try to fit into them
  2. if it was unsuccessful then describe the type based on given input (right side of assignment)
  3. if there were any constraints and types don't match then present them as diffs in error message

So, instead of treating [] as Array(a) it should recursively match to a constraint, because the a may be already defined. If there is no constraint then it would end up with Array(a). However, currently we HAVE to explicitly define types for state variables (and everything else) so we do have the constraints, am I right?

@gdotdesign
Copy link
Member

gdotdesign commented Feb 18, 2020

Thank you for the explanation, really appreciate it 👍

You are right, we can and should make better error messages if possible, in this case it is certainly possible, I just wanted to point out that it would not solve the underlying issue.

After thinking about it we can try not to resolve sub fields records to types and can just use their type signatures to match the parent record to a type, but in this case it would not work I am afraid since as you wrote an empty array resolves as Array(a), to resolve them with different type variables a, b we need to track them during the resolving of a record which would increase complexity and there could be a case where it's counter intuitive (you explicitly want them to be the same type variable).

I like having [] as Array(a) or [] of A as optional so it would not be a breaking change.

I suggest to keep this issue for having a nicer error message for states and properties if they are records, and make an other for the underlying issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Development

No branches or pull requests

3 participants