Page MenuHomePhabricator

[Sema] Put nullability fix-it after the end of the pointer.

Authored by vsapsai on Sep 27 2017, 12:33 PM.



Fixes nullability fix-it for id<SomeProtocol>. With this change
nullability specifier is inserted after ">" instead of between
"id" and "<".


Diff Detail


Event Timeline

vsapsai created this revision.Sep 27 2017, 12:33 PM
jordan_rose accepted this revision.Sep 27 2017, 12:37 PM

Nice catch, Volodymyr! Looks good to me.

This revision is now accepted and ready to land.Sep 27 2017, 12:37 PM

To preempt some of review feedback here are attempted and rejected approaches:

  • Pass pointerLoc and pointerEndLoc as pointerRange. Such source range can be misleading as pointerLoc doesn't necesserily point at the beginning. For example, for int *x pointer range would be "*" and I would expect it to be "int *". So it's not really a range but 2 related locations.
  • Use D.getDeclSpec().getLocEnd() instead of D.getDeclSpec().getTypeSpecTypeLoc(). In this case warning location points at the closing angle bracket and that can be confusing to developers. It looks like
./test.h:14:3: warning: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)
  id<SomeProtocol> thingies;
  • Use pointerLoc for insert note instead of pointerEndLoc. It looks like
./test.h:14:3: note: insert '_Nullable' if the pointer may be null
  id<SomeProtocol> thingies;

compared to suggested

./test.h:14:18: note: insert '_Nullable' if the pointer may be null
  id<SomeProtocol> thingies;

I don't expect developers to know that they should match whitespace preceding _Nullable to calculate where in the line it should be inserted. And I think developers shouldn't care about it. So put the cursor where you expect the text to be inserted.

This revision was automatically updated to reflect the committed changes.

Nice catch, Volodymyr! Looks good to me.

Thanks for the quick review, Jordan.