Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Parse] Introduce /.../ regex literals #42119

Merged
merged 6 commits into from
Apr 12, 2022

Conversation

hamishknight
Copy link
Contributor

@hamishknight hamishknight commented Mar 31, 2022

Start parsing regex literals with /.../ delimiters when -enable-bare-slash-regex is passed.

rdar://83253726

@hamishknight
Copy link
Contributor Author

Source compat was run with it enabled in #41767

@hamishknight
Copy link
Contributor Author

@xedin @beccadax Mind taking a look at the DiagnosticEngine part?

lib/Parse/Lexer.cpp Outdated Show resolved Hide resolved
class DiagnosticQueue final {
/// The underlying diagnostic engine that the diagnostics will be emitted
/// by.
DiagnosticEngine &UnderlyingEngine;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me propose an alternate design for DiagnosticQueue:

  • There is no UnderlyingEngine member
  • DiagnosticQueue::DiagnosticQueue() just adds a ForwardingDiagnosticConsumer to QueueEngine
  • emit() just closes the QueueEngine's transaction, flushing the tentative diagnostics out to consumers

I think that would mean you wouldn't have to modify DiagnosticEngine at all. Does your design have advantages over that one? (It might—I'd like to hear about them.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I did play about with ForwardingDiagnosticConsumer a bit, the main issue is that is forwards directly to the consumers and bypasses the target engine altogether, so doesn't update any state on the engine (including hadAnyError!). Now, we should definitely fix that, although it's not clear to me whether all of its clients actually want the diagnostic to be fully re-evaluated by the target engine. There are a couple of other things:

  • We'd need to turn a DiagnosticInfo back into a Diagnostic to be re-evaluated by the target engine (as it may need to become a tentative diagnostic). We'd need special handling to make sure e.g warning-as-error is preserved if the original diagnostic engine had it set.
  • Because we'd be emitting the diagnostic in a dummy engine, we'd lose the caching for decl rendering (though we probably ought to cache that on the ASTContext or in the evaluator).

I don't think there's anything here that can't be solved, but it seemed less clean to me than creating a dummy diagnostic engine to effectively serve as a Diagnostic builder, which can then be passed onto a target engine. If we want ForwardingDiagnosticConsumer to properly support being able to forward-and-re-evaluate a diagnostic, then I agree we should change this to do that. But either way we'll need DiagnosticEngine changes I believe.

lib/AST/DiagnosticEngine.cpp Outdated Show resolved Hide resolved
include/swift/AST/DiagnosticEngine.h Outdated Show resolved Hide resolved
lib/Parse/ParseExpr.cpp Outdated Show resolved Hide resolved
include/swift/Parse/Parser.h Outdated Show resolved Hide resolved
lib/Parse/Lexer.cpp Outdated Show resolved Hide resolved
@hamishknight hamishknight force-pushed the regular-grammar branch 2 times, most recently from 11ad996 to 0d59e88 Compare April 12, 2022 14:35
This allows us to hold tentative diagnostics
independent of other diagnostic transactions.
Queue up diagnostics when lexing, waiting until
`Lexer::lex` is called before emitting them. This
allows us to re-lex without having to deal with
previously invalid tokens.
Change the flag to imply
'-enable-experimental-string-processing', and
and update some tests to start using it.
When forward slash regex is enabled, start emitting
an error on prefix operators containing the
`/` character.
This spelling is no longer used.
Start parsing regex literals with `/.../`
delimiters.

rdar://83253726
@hamishknight hamishknight force-pushed the regular-grammar branch 2 times, most recently from 0d59e88 to 11ad996 Compare April 12, 2022 15:04
@hamishknight
Copy link
Contributor Author

I tried to be clever with the force pushing, but GitHub screwed up the timestamps so the above "force pushed" link is wrong. Correct diff is https://github.com/apple/swift/compare/11ad996fe954ee73c4e63b90cfaec1efed2ae23b..f1a799037e3d00ea530480a1d67c281524a29a42.

Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thank you!

@hamishknight
Copy link
Contributor Author

@swift-ci please smoke test Linux

@hamishknight hamishknight merged commit 28bd13d into swiftlang:main Apr 12, 2022
@hamishknight hamishknight deleted the regular-grammar branch April 12, 2022 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants