This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Code action for creating an ObjC initializer
ClosedPublic

Authored by dgoldman on Dec 29 2021, 1:26 PM.

Details

Summary

The code action creates an initializer for the selected
ivars/properties, defaulting to all if only the interface/implementation
container is selected.

We add it based on the position of the first non initializer that we
see, and default to adding it where the @end token is.

We also use the ObjC parameter form of (nullable id) instead of
(id _Nullable) if the property has the nullable attribute.

Diff Detail

Event Timeline

dgoldman created this revision.Dec 29 2021, 1:26 PM
dgoldman requested review of this revision.Dec 29 2021, 1:26 PM
dgoldman updated this revision to Diff 396569.Dec 29 2021, 1:30 PM

Remove debug log statement

dgoldman updated this revision to Diff 396676.Dec 30 2021, 8:03 AM

Minor change to how we get the loc of @end

dgoldman updated this revision to Diff 396706.Dec 30 2021, 12:29 PM

Add another minor test case

This looks like a useful feature, I had been thinking about adding the equivalent for C++...

High level feedback:

  • this should generate both the declaration and definition
  • i doubt the interaction around subsetting fields is going to be satisfying
  • don't spend complexity on lots of options/micromanaging formatting until basic functionality is solid
clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp
67

If we're going to care about this (which makes sense), this should be a rangeIncludingComment() in AST.h or so

86

the two returns are inconsistent, this one is the first position outside the range, the other return is the last token (not character) in the range

92

We try not to spend complexity on formatting beyond what clang-format cares about.

If you're inserting before a member (or @end), always inserting 2 newlines seems close enough? (clang-format will strip consecutive blank lines)

114

This algorithm doesn't make much sense to me.
You want to insert before the first non-init instance method, falling back to the end.
So why do you need to do more than search for the first non-init instance method, and find the location before it? In particular why are we looking at init methods and locations after them?

142

Name.consume_front("_"); (FWIW I don't think this needs a comment)

200

I understand the desire to select the particular fields, but given the limitation of the interaction I wonder whether these cases are useful & discoverable enough to be worth the complexity.

  • how often is an initializer that sets one variable of many useful?
  • out of the times when a subset of variables should be initialized, how often are they contiguous in the code?

I think it would likely be better to start with initializing all members...

219

while here, also grab the InterfaceDecl into another field so you don't have to go casehunting again in initParams?

225

can you rename this method to something clearer?
unclear whether "init" stands for "initialize" or "initializer" here, which would refer to pretty different things!

252

Inferring the container was selected by looking at HadBadNode/Params seems very indirect.
Why not handle this case at the top? if (Container == N->ASTNode.get<ObjcContainerDecl>()) return getAllParams(Iface)

283

!Container isn't possible here

286

nit: why isn't this a return value?

291

Wait, so if the user triggers the action on @interface then they'll get a decl with no impl, and if they trigger on @implementation they'll get the opposite?

Seems like we should at least try to generate both, no? (May not be possible if we're triggering on @interface and index lookup fails, but still...)

292

For building strings like this with formatting, we usually use raw_string_ostream and formatv.
This would make the stuff starting at 303 easier to read

296

This is hard to read, use a raw string

299

nit: why braces? (and elsewhere)

321

you need to verify that Loc is actually in the main file :-(

dgoldman updated this revision to Diff 397321.Jan 4 2022, 9:28 AM
dgoldman marked 11 inline comments as done.

Some review fixes, still need to rebase for new changes

dgoldman marked 3 inline comments as not done.Jan 4 2022, 9:30 AM
dgoldman added inline comments.
clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp
114

Yeah, I think that makes more sense, changed.

200

I didn't realize that VS Code only lets you select a range, not selecting multiple points (e.g. multiple cursors). I still think this could be useful and it isn't that hard to support, although yes, ideally we could do something like IntelliJ where it presents you a UI to select fields. The main use case of this atm is for simple model objects, and for those I think it could be useful to make some params optional although the contiguous limitation is definitely restrictive.

FWIW I don't think code actions in general are particularly discoverable.

286

IIUC this (having the impl vector be an out param) seems to be the convention - SmallVectorImpl(const SmallVectorImpl &) = delete;, but I can change the return type to instead be SmallVector<MethodParameter, 8>, WDYT?

291

Yeah, I decided to avoid this due to the complexity for a v1, could look into adding it in a follow up. For sourcekit-lsp, it would only work if you've opened the impl first (since it will only have the dynamic index available). Is there anything I could look at which does something similar?

292

Yep that helps quite a bit

dgoldman updated this revision to Diff 397329.Jan 4 2022, 10:05 AM
dgoldman marked 3 inline comments as done.

Rebase + use new helpers

dgoldman marked an inline comment as done.Jan 4 2022, 10:07 AM
dgoldman added inline comments.
clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp
92

Just went with that for now, can revisit later.

Alright, the implementation looks good.

I think just a question of what to do when firing on @implementation. I'd suggest one of:

  • generating the interface too, you have almost allthe code
  • not supporting @implementation yet

We can also keep the current behavior if you feel strongly about it, but I guess it will confuse people.

clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp
266

If there are no non-init instance methods this will insert at the very bottom of the @implementation. You can add a second anchor to cover this case.

A policy that might make sense here is:

{{IsInitMethod, Below}, {Anything, Above}}

But up to you

286

Please do.

There are a bunch of reasons that vector out-params are sometimes used in LLVM:

  • caller rather than callee should choose the SmallVector size
  • repeated calls to append to an existing vector
  • return value is something else
  • some old pre-C++11 code

but I think none apply here

291

This seems like it's going to be a surprising limitation. You can mark it the tweak as "hidden" if you don't want to expose it to users yet.

Is there much complexity in generating both if targeting the @implementation? It seems you've already got both codepaths and just need to run both. If that's too complicated, it doesn't really seem like a good idea to support this on implementations, generating a definition for a member that doesn't exist is confusing.

(I'd suggest changing the description to "declare" memberwise initializer whenever you're not generating the definition.)

For sourcekit-lsp, it would only work if you've opened the impl first

The general approach is to find the target file and then pseudoparse the bits you need, as you won't have an AST in any case. So it will still work without index if the file can be found using filename heuristics.

Is there anything I could look at which does something similar?

DefineOutline does something similar: it triggers on a declaration and edits the declaration and inserts a definition.

dgoldman updated this revision to Diff 397956.Jan 6 2022, 11:58 AM
dgoldman marked an inline comment as done.

Support generating interface and impl

dgoldman marked 3 inline comments as done.Jan 6 2022, 12:01 PM
dgoldman added inline comments.
clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp
266

I think this should be fine to start off, we probably want to be after class methods.

291

I changed it to add the declaration and impl if you select the implementation, could also do the same for selecting the interface if the impl is still visible, WDYT? Also had to change some editing logic, LMK what you think.

dgoldman marked an inline comment as done.Jan 6 2022, 12:28 PM

PTAL, let me know what you think regarding the edits, had to make sure it works even if we generate multiple edits in the same file.

sammccall added inline comments.Jan 31 2022, 8:25 AM
clang-tools-extra/clangd/refactor/Tweak.h
85 ↗(On Diff #397956)

I don't think mergingEdit is a great API (a few issues, including not enough use cases to know if it's right).

Rather than try to polish it here, can we just simulate it in the caller? DefineInline.cpp has an example of this.

dgoldman updated this revision to Diff 405765.Feb 3 2022, 1:19 PM

mergingEdit --> manual file checks

dgoldman marked an inline comment as done.Feb 3 2022, 1:19 PM

friendly ping

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 3:04 PM
sammccall accepted this revision.Mar 17 2022, 6:43 AM

Thanks, and sorry for the long delay.

clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp
282

Not sure I see the point of using a SmallVector here instead of writing directly into Effect.ApplyEdits?

295

Maybe a comment like:

// If we see the class implementation, add the initializer there too.
// FIXME: merging the edits is awkward, do this elsewhere

(I don't think we should fix it in this patch, but we should at some point)

301

nit: I'd invert the condition here, to avoid the "else" block being a double-negative

This revision is now accepted and ready to land.Mar 17 2022, 6:43 AM
dgoldman updated this revision to Diff 416186.Mar 17 2022, 8:23 AM
dgoldman marked 3 inline comments as done.

Fixes for review

This revision was landed with ongoing or failed builds.Mar 17 2022, 8:32 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Mar 17 2022, 9:01 AM

Looks like this breaks tests: http://45.33.8.238/linux/71327/step_9.txt

Please take a look and revert for now if it takes a while to fix