This is an archive of the discontinued LLVM Phabricator instance.

Add support for editor placeholders to Clang's lexer
ClosedPublic

Authored by arphaman on Apr 14 2017, 6:46 AM.

Details

Summary

This patch is based on the RFC that I've sent out earlier (http://lists.llvm.org/pipermail/cfe-dev/2017-April/053394.html).
It teaches Clang's Lexer to recognize the editor placeholders that are produced by some editors like Xcode when expanding code-completion results that include placeholders. When the Lexer sees a placeholder, it emits an error 'editor placeholder in source file', suppresses all diagnostics in the placeholder range and lexes the tokens in the placeholder.

This patch also includes a new compiler option called '-fsuppress-editor-placeholder-error' that surpasses the 'editor placeholder in source file' error. This option is useful for an IDE like Xcode as we don't want to display those errors as live diagnostics in the editor.

There are still certain things that can be improved. For example, in the future we may want to add support for different kinds of placeholders (e.g. typed, it would produce special typed expressions) that would actually produce a special token instead of lexing the contents of the placeholder in order to improve the parser's recovery. But they will require changes to the code-completion code as well, so I left them for later to keep this patch small.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Apr 14 2017, 6:46 AM
arphaman updated this revision to Diff 95297.Apr 14 2017, 7:41 AM

I've decided to treat each placeholder as an identifier token (like Swift) instead of just lexing the contents. This will prevent spurious errors for declaration name placeholders that have spaces or hyphens inside.

benlangmuir edited edge metadata.Apr 14 2017, 11:27 AM

Rather than stick ranges into the diagnostic engine, I think it would be cleaner to have the identifier have a method like isEditorPlaceholder() that can be used to avoid situations like this in a principled way, or to customize behaviour for placeholders in the parser, etc. That's how we are handling it in Swift. Using an API on the placeholder is also better for handling errors that could be caused by the placeholder but not have it as the primary location.

What do you think?

include/clang/Driver/Options.td
1473 ↗(On Diff #95297)

How about -fallow-editor-placeholder? It's a little shorter, and not specific to a single error.

Rather than stick ranges into the diagnostic engine, I think it would be cleaner to have the identifier have a method like isEditorPlaceholder() that can be used to avoid situations like this in a principled way, or to customize behaviour for placeholders in the parser, etc. That's how we are handling it in Swift. Using an API on the placeholder is also better for handling errors that could be caused by the placeholder but not have it as the primary location.

What do you think?

I thought about this as well. I'm not sure which way is better though. The current way is simple in the sense that we automatically suppress all diagnostics in the placeholder, so we don't have to modify the parser at all. Clang generates placeholders in a bunch of different places so I reckon there'll have to be a lot of modifications to suppress all problematic cases. Although I suppose that if we teach the parser about the most common places where placeholders could occur (e.g. expressions), that would probably suppress the majority of diagnostics that we care about. I'll try out the parser changes right now.

Rather than stick ranges into the diagnostic engine, I think it would be cleaner to have the identifier have a method like isEditorPlaceholder() that can be used to avoid situations like this in a principled way, or to customize behaviour for placeholders in the parser, etc. That's how we are handling it in Swift. Using an API on the placeholder is also better for handling errors that could be caused by the placeholder but not have it as the primary location.

What do you think?

I thought about this as well. I'm not sure which way is better though. The current way is simple in the sense that we automatically suppress all diagnostics in the placeholder, so we don't have to modify the parser at all. Clang generates placeholders in a bunch of different places so I reckon there'll have to be a lot of modifications to suppress all problematic cases. Although I suppose that if we teach the parser about the most common places where placeholders could occur (e.g. expressions), that would probably suppress the majority of diagnostics that we care about. I'll try out the parser changes right now.

It seems that suppressing placeholders in expressions and type names would cover the a lot of the common problematic cases. What do you think about a hybrid approach: Don't suppress diagnostics in the placeholder range when we handle the placeholders in parser/sema, but do suppress the range when the placeholder isn't explicitly handled?

What do you think about a hybrid approach: Don't suppress diagnostics in the placeholder range when we handle the placeholders in parser/sema, but do suppress the range when the placeholder isn't explicitly handled?

I'd really rather not plumb this into the diagnostics engine at all if we can avoid it. Are there cases we somehow cannot handle using the explicit checks, or is it just a question of finding all the cases?

It's more about handling all of the cases, e.g. the completion results for declaration patterns with placeholders, as those can't be caught with a check at an expression/typename level.

I guess then I can catch all of the most common cases now and we can improve the suppression for the remaining cases later on.

arphaman updated this revision to Diff 95348.Apr 14 2017, 2:42 PM
arphaman marked an inline comment as done.
  • Don't use the diagnostic engine suppression ranges, but rely on changes to parser/sema for diagnostic suppression.
  • Rename the compiler flag to '-fallow-editor-placeholders'.
arphaman updated this revision to Diff 95349.Apr 14 2017, 2:51 PM

Check if IdentifierInfo is nil in ActOnIdExpression to prevent crash.

benlangmuir added inline comments.Apr 18 2017, 8:48 AM
include/clang/Basic/IdentifierTable.h
358 ↗(On Diff #95349)

Nitpick: There should probably be an example in the doc comment. The editor placeholder syntax isn't commonly known.

include/clang/Driver/Options.td
1478 ↗(On Diff #95349)

Does the negative -fno- option need to be CC1Option? Later in this patch you seem to canonicalize on the positive form when passing to the frontend.

arphaman updated this revision to Diff 95596.Apr 18 2017, 10:10 AM
arphaman marked 2 inline comments as done.

Add comments and remove -fno-allow-editor-placeholders from CC1 options.

benlangmuir accepted this revision.Apr 18 2017, 10:11 AM
This revision is now accepted and ready to land.Apr 18 2017, 10:11 AM
This revision was automatically updated to reflect the committed changes.