Skip to content

Conversation

nkcsgexi
Copy link
Contributor

If syntax trees are requested, we shouldn't skip inactive code. Notice the
inactive code won't be skipped in SwiftSyntax because we always set
PerformConditionEvaluation false for the in-process parser.

This is mostly needed for testing purposes where we add -verify-syntax-tree
to regular compiler invocations.

rdar://50837165

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi nkcsgexi requested a review from rintaro May 16, 2019 23:18
@@ -19,3 +19,7 @@ class { // expected-error {{unknown declaration syntax exists in the source}}
// expected-error@-4 {{top-level statement cannot begin with a closure expression}}

}

#if swift(<1)
print("Wat")
Copy link
Member

Choose a reason for hiding this comment

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

Am I mistaken, or does this really not actually verify anything? It would require that the user manually verify that the inactive region is processed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This verifies we can get syntax nodes for inactive code like print("wat"), otherwise an error message will be emitted to complain about unknown syntax nodes.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about we add an extra step to sanity check that everything is parsed and round-tripped ?
E.g. add a RUN line to check that printing the syntax-tree will result in the same output as the input source file.
This will ensure that everything is parsed as expected.

rintaro
rintaro previously approved these changes May 17, 2019
@rintaro rintaro self-requested a review May 17, 2019 00:47
@rintaro rintaro dismissed their stale review May 17, 2019 00:48

Mistake

// We shouldn't skip code if we are building syntax tree.
// The parser will keep running and we just discard the AST part.
SmallVector<ASTNode, 16> dropedElements;
parseElements(dropedElements, false);
Copy link
Member

Choose a reason for hiding this comment

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

Can we disable diagnostics using DiagnosticSuppression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@akyrtzi @rintaro Two additional commits were added to disable diagnostics in the inactive code though we parse inside the inactive code. Notice the existing design of syntax tree verifier works as a post-processor. To make it compatible with DiagnosticSuppression, we have to refactor the verifier to as on-the-fly.

@jrose-apple
Copy link
Contributor

I'm a little concerned about this since we just saw issues with #if canImport(…) getting evaluated in inactive blocks. What keeps something like that from happening here?

@nkcsgexi nkcsgexi force-pushed the syntax-tree-with-condition branch from 8ad1006 to ad4a4dc Compare May 17, 2019 18:40
@akyrtzi
Copy link
Contributor

akyrtzi commented May 17, 2019

I think that it shouldn't be the responsibility of SyntaxTreeCreator to emit verification diagnostics, I preferred it when the SyntaxVerifier was a separated, self-contained, functionality.

What do you think about an alternative solution; we could just disable the syntax-tree verifier in the case where #if swift showed up, which is pretty uncommon.
Instead of:

} else if (SyntaxContext->isEnabled()) {
  // We shouldn't skip code if we are building syntax tree.
  // The parser will keep running and we just discard the AST part.
  DiagnosticSuppression suppression(Context.Diags);
  SmallVector<ASTNode, 16> dropedElements;
  parseElements(dropedElements, false);
} else {

we'd do:

} else if (SyntaxContext->isEnabled()) {
  // Disable the syntax tree verifier, we are skipping parsing, so 'unknown' node is expected.
  <indicate that syntax tree verifier must not run>
} else {

This has these benefits:

  • Preserves the existing behavior, which is that no parsing should occur
  • Much simpler.

nkcsgexi added 2 commits May 17, 2019 12:32
If syntax trees are requested, we shouldn't skip inactive code. Notice the
inactive code won't be skipped in SwiftSyntax because we always set
PerformConditionEvaluation false for the in-process parser.

This is mostly needed for testing purposes where we add -verify-syntax-tree
to regular compiler invocations.

rdar://50837165
@nkcsgexi nkcsgexi force-pushed the syntax-tree-with-condition branch from ad4a4dc to f25925f Compare May 17, 2019 19:37
@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@swift-ci Please clean test Linux platform

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 7af15fab462f65fafe1929455570662d99b13f6a

@nkcsgexi
Copy link
Contributor Author

@swift-ci Please smoke test OS X platform

@nkcsgexi nkcsgexi merged commit d05a748 into swiftlang:master May 20, 2019
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.

6 participants