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

Method not found when type checking Table stemming from incomplete set of methods in global provides #1693

Open
robertDurst opened this issue Jun 13, 2023 · 8 comments

Comments

@robertDurst
Copy link
Contributor

Consider the following code:

tbl = table: name
  row: "Rob"
end

tbl.row("Foo")

When I execute without type checker all is well.
Screenshot 2023-06-13 at 12 45 32 AM

With type checker, .row is not found.
Screenshot 2023-06-13 at 12 45 45 AM

I did quite a bit of digging and printing and I noticed that the fields on the Table data-type obeject resulting from instantiate-data-type(obj-type, context) were surprisingly limited:

t-data(Table, [list: ], [list: ], [string-dict: length, ( -> Number)], builtin(dummy location))

I expect many more methods than just this on Table. Eventually I landed on the global provides and noticed this:

"Table": ["data", "Table", [], [], {
    "length": ["arrow", [], "Number"]
}],

I reproduced locally by modifying tests/type-check/good/table.arr, then added the type signature for row to the global provides and it passed the type checker.

I noticed String-Dict and Reactor both "provide" their own data-types. Should Table do the same?

60 seconds of ctrl-f made me realize just moving the data-type from global provides to table provides isn't as simple as it might seem... it seems it will require resolving some dependency/module loading. Wanted to confirm the desired behavior before venturing further :)

@shriram
Copy link
Member

shriram commented Jun 13, 2023

I think the issue is that table type-checking has not really been touched in general. What is the return type of some of these methods? This is in some sense the entire research problem that inspired B2T2.

@robertDurst
Copy link
Contributor Author

This is in some sense the entire research problem that inspired B2T2.

Ahh, yes. Well I have now just about gone full circle. B2T2 was my starting point.

What is the return type of some of these methods?

That is a good question. I was looking at some of the type signatures for Table methods in the docs that didn't quite line up with my understanding and, unironically, discussing this with @bennn last night.

While the one hardcoded method (length) is valid, is it worth moving this datatype to the Table module preceding the table type work or as a follow up?

My opinion is maybe 🤷 . However, we don't really get much until we make progress on the table type work.

@shriram
Copy link
Member

shriram commented Jun 13, 2023

My understanding is that Ben and you plan to tackle this, which would be great. I'm not sure a half-baked solution is worth much; we can't use a "half typed" program. I'd say take it on and do it thoroughly. But maybe @jpolitz @blerner want to weigh in.

(Btw, coincidence, not irony.)

@jpolitz
Copy link
Member

jpolitz commented Jun 13, 2023

Yeah, there needs to be design of types for Table<... very interesting stuff ...> and Row<... very interesting stuff ...> before this will make much sense statically. Excited to work on it!

@bennn
Copy link
Member

bennn commented Jun 13, 2023

Really what we're looking for now is a "good first issue" for Rob to build some familiarity with the Pyret typechecker. Rounding out the boring types (Table, Row) to match the Pyret docs seemed like a good plan to me.

Can you suggest another issue to try? Maybe #1662 ?

@jpolitz
Copy link
Member

jpolitz commented Jun 14, 2023

I see! That context is helpful.

Certainly it's probably better for this program to succeed and have some kind of any-ish Row type, so that's a good place to poke around to do some learning, good idea.

I suspect that the type used for Table here won't resemble the type you end up with, but that's fine.

@robertDurst
Copy link
Contributor Author

On a related note, linking #1422 (linked to #1662) since it looks like it's the same issue described above.

First I fixed the checkWrapTable error by adding its type default to type-defaults.arr (just like checkWrapBoolean). This resulted in a net new error:

  Values not equal "./tests/type-check/good/reactor.arr should not have compilation errors: " "The type checker rejected your program because the object type  \"Table\"  at  tc-test://reactor.arr:5:11-10:3  does not have a field named  `_column-index`  as indicated by the access of that field at  <builtin dummy location>"

which ultimately points back to the global provides.

@bennn
Copy link
Member

bennn commented Jun 19, 2023

Interesting! I sure hope work on this issue helps the others.

Reactor sounds like a good example to follow for moving types from global provides to Table provides.

@jpolitz @blerner any tips for adding types to table provides?

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

No branches or pull requests

4 participants