You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/03/12 04:39:15 UTC
[GitHub] [tvm] slyubomirsky opened a new pull request #7647: [Relay][bugfix] Do not allow an empty match block to type check
slyubomirsky opened a new pull request #7647:
URL: https://github.com/apache/tvm/pull/7647
Currently, the type checker returns an `IncompleteTypeNode` for a match expression that has no clauses. This means the incomplete type node can be unified with another type and be valid even though a match expression with no clauses has no defined semantics and shouldn't be permitted. I think it would be sensible for the type checker to disallow match expressions with no clauses.
Incidentally, the match completeness checker would normally prevent an empty match expression, but they can still pass the type checker if the `complete` flag is set to off. Hence a function like this would type check even though it makes no sense: `def @main() -> fn () -> fn ( ()) -> bool { match? 1 {}}`
This PR checks for empty match expressions and produces a type checking error. Bug discovered by fuzzing thanks to @Flandini
Please review @jroesch @MarisaKirisame
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7647: [Relay][bugfix] Do not allow an empty match block to type check
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7647:
URL: https://github.com/apache/tvm/pull/7647#discussion_r592919396
##########
File path: tests/python/relay/test_type_infer.py
##########
@@ -416,6 +416,21 @@ def test_dynamic_function():
assert mod["main"].params[0].checked_type == s_tt
+@pytest.mark.xfail(raises=tvm.error.TVMError)
Review comment:
let's use `with pytest.raises` instead (https://docs.pytest.org/en/stable/assert.html#assertions-about-expected-exceptions), because the testcase tests for throwing an exception, not a known limitation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] MarisaKirisame commented on pull request #7647: [Relay][bugfix] Do not allow an empty match block to type check
Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on pull request #7647:
URL: https://github.com/apache/tvm/pull/7647#issuecomment-797647424
It has semantic exactly just like a partial match that fail - it is an runtime error. I dont think we should have a special case for 0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] slyubomirsky edited a comment on pull request #7647: [Relay][bugfix] Do not allow an empty match block to type check
Posted by GitBox <gi...@apache.org>.
slyubomirsky edited a comment on pull request #7647:
URL: https://github.com/apache/tvm/pull/7647#issuecomment-797803487
Yeah the error only triggers if `complete` is set to false (that's how the fuzzer found it)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] slyubomirsky commented on a change in pull request #7647: [Relay][bugfix] Do not allow an empty match block to type check
Posted by GitBox <gi...@apache.org>.
slyubomirsky commented on a change in pull request #7647:
URL: https://github.com/apache/tvm/pull/7647#discussion_r592924209
##########
File path: tests/python/relay/test_type_infer.py
##########
@@ -416,6 +416,21 @@ def test_dynamic_function():
assert mod["main"].params[0].checked_type == s_tt
+@pytest.mark.xfail(raises=tvm.error.TVMError)
Review comment:
Good to know, I was wondering what the right way to do that was
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] slyubomirsky commented on pull request #7647: [Relay][bugfix] Do not allow an empty match block to type check
Posted by GitBox <gi...@apache.org>.
slyubomirsky commented on pull request #7647:
URL: https://github.com/apache/tvm/pull/7647#issuecomment-797654917
So if we don't consider it a bug, an empty match expression will always fail at run time and will always type check, so it's effectively the error node that we discussed having. Do we have the semantics of a failed match defined anywhere? It might be good to have a uniform interface/expectations for that
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] slyubomirsky commented on pull request #7647: [Relay][bugfix] Do not allow an empty match block to type check
Posted by GitBox <gi...@apache.org>.
slyubomirsky commented on pull request #7647:
URL: https://github.com/apache/tvm/pull/7647#issuecomment-797803487
Yeah the bug only triggers if `complete` is set to false (that's how the fuzzer found it)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] MarisaKirisame commented on pull request #7647: [Relay][bugfix] Do not allow an empty match block to type check
Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on pull request #7647:
URL: https://github.com/apache/tvm/pull/7647#issuecomment-797722870
Yes that would be an error. There is also the possibility that your ADT is void (has no clause) and match simply match that. IIRC the match node has a 'must_be_exhaustive' flag, so the codegen emitting this should simply turn the flag on.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] slyubomirsky closed pull request #7647: [Relay][bugfix] Do not allow an empty match block to type check
Posted by GitBox <gi...@apache.org>.
slyubomirsky closed pull request #7647:
URL: https://github.com/apache/tvm/pull/7647
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] slyubomirsky commented on pull request #7647: [Relay][bugfix] Do not allow an empty match block to type check
Posted by GitBox <gi...@apache.org>.
slyubomirsky commented on pull request #7647:
URL: https://github.com/apache/tvm/pull/7647#issuecomment-798805017
I'll withdraw the PR because I agree that we probably shouldn't consider this a bug if we allow some matches to miss regardless.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org