Skip to content

Conversation

@joshpoll
Copy link
Contributor

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from others in the community.

@slyubomirsky
Copy link
Contributor

slyubomirsky commented Oct 15, 2018

Good catch. Be sure to change the test for it as well.

@joshpoll joshpoll mentioned this pull request Oct 15, 2018
3 tasks
@tqchen
Copy link
Member

tqchen commented Oct 15, 2018

I am not completely sure this should be the semantics for every alpha equal checking, as the same incomplete type appearing in similar locations naturally has certain meanings.

@joshpoll
Copy link
Contributor Author

@tqchen I don't quite understand. Do you have specific examples where this is not the desired behavior? Both @MarisaKirisame and I have cases where this PR gives us the desired behavior.

@tqchen
Copy link
Member

tqchen commented Oct 15, 2018

The question is that whether the following two code alpha-equals to each other, note that first one have shares ```?a`` among two type annotations and that could potentially represents a type constraint.

let x: ?a = 1;
let y: ?a = 2
x
let x: ?a = 1
let y: ?b = 2
x

@slyubomirsky
Copy link
Contributor

slyubomirsky commented Oct 15, 2018

Apologies for my inattentive reading, I thought the initial change was adding a check for kind, not eliminating the pointer equality check. I agree that two different type vars should not be considered alpha-equal simply because they have the same kind (perhaps we could do the same thing as with ordinary vars as in #1900?)

@MarisaKirisame
Copy link
Contributor

@joshpoll I had been using incompletetypenode to signal 'infer for me'.
I switch to None and all is fine.

@joshpoll
Copy link
Contributor Author

If we can use None instead of IncompleteType nodes, what is the purpose of the latter? Are they not meant to be specified by users but only used internally by the type checker? If that's the case, perhaps we should make a stronger/clearer distinction between these two types of nodes.

@joshpoll
Copy link
Contributor Author

Looks like using None for a function return type causes a nullptr exception during alpha equality checking?

@joshpoll
Copy link
Contributor Author

As long as we allow nullptrs in function return types and in variable types, I think I can fully switch my annotations to None instead of IncompleteType. I can close this PR and submit a new one with changes to alpha equality that add more type defined() checks if that sounds good to you all. I've already made the change in the text format branch.

@tqchen
Copy link
Member

tqchen commented Oct 15, 2018

Supporting None sounds good

@joshpoll joshpoll closed this Oct 15, 2018
@joshpoll
Copy link
Contributor Author

I think we still need to revise the semantics of alpha equality for IncompleteTypes, though, like Steven suggested, since with just pointer equality two structurally identical expressions aren't alpha equal.

@joshpoll joshpoll deleted the patch-1 branch October 15, 2018 22:38
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