[jit] Fix type hints for None constants #23029
Conversation
jamesr66a
left a comment
jamesr66a
left a comment
There was a problem hiding this comment.
Looks OK but I had some questions
| n->output()->setType(result_type); | ||
| } else { | ||
| // Implicitly wrap non-optionals | ||
| n->output()->setType(OptionalType::create(result_type)); |
There was a problem hiding this comment.
How can this branch ever be reached ? inferred_type->isSubtypeOf(NoneType::get()) && !inferred_type->isSubtypeOf(result_type)) means that result_type has to be an Optional.
There was a problem hiding this comment.
This is in there to maintain compatibility for calls like torch.jit.annotate(Tensor, None) where the type hint is Tensor
There was a problem hiding this comment.
I think this should be handled in the compiler and not here. InsertConstant is a common API and this usage would be a bug at any of the other callsites
There was a problem hiding this comment.
It's not a bug though, it's something we support currently. Plus keeping it here lets us have all usages in 1 place so we can more easily delete it later if we go that route
There was a problem hiding this comment.
It is a bug. In any other case having a type of Tensor for a value of None is a bug
The type hint was being ignored when emitting
Noneconstants, this also de-dups some testing codeDifferential Revision: D16364572