From c241e782e03f2451d1228c1eb6be596c5661b343 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Sat, 17 Aug 2024 22:45:55 -0700 Subject: [PATCH 1/3] Use SwiftIfConfig to determine where to emit new parser diagnostics --- include/swift/Bridging/ASTGen.h | 1 + lib/ASTGen/Sources/ASTGen/SourceFile.swift | 28 ++++++++++------------ lib/Parse/ParseDecl.cpp | 4 ++-- test/ASTGen/if_config.swift | 4 ++-- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/include/swift/Bridging/ASTGen.h b/include/swift/Bridging/ASTGen.h index bb09e8b0097c6..19cef55aef530 100644 --- a/include/swift/Bridging/ASTGen.h +++ b/include/swift/Bridging/ASTGen.h @@ -48,6 +48,7 @@ int swift_ASTGen_roundTripCheck(void *_Nonnull sourceFile); /// Emit parser diagnostics for given source file.. Returns non-zero if any /// diagnostics were emitted. int swift_ASTGen_emitParserDiagnostics( + BridgedASTContext astContext, void *_Nonnull diagEngine, void *_Nonnull sourceFile, int emitOnlyErrors, int downgradePlaceholderErrorsToWarnings); diff --git a/lib/ASTGen/Sources/ASTGen/SourceFile.swift b/lib/ASTGen/Sources/ASTGen/SourceFile.swift index 927153b571b8d..4c9dc3e2bbe8b 100644 --- a/lib/ASTGen/Sources/ASTGen/SourceFile.swift +++ b/lib/ASTGen/Sources/ASTGen/SourceFile.swift @@ -12,6 +12,7 @@ import ASTBridging import SwiftDiagnostics +import SwiftIfConfig @_spi(ExperimentalLanguageFeatures) import SwiftParser import SwiftParserDiagnostics import SwiftSyntax @@ -142,20 +143,10 @@ public func roundTripCheck( } } -extension Syntax { - /// Whether this syntax node is or is enclosed within a #if. - fileprivate var isInIfConfig: Bool { - if self.is(IfConfigDeclSyntax.self) { - return true - } - - return parent?.isInIfConfig ?? false - } -} - /// Emit diagnostics within the given source file. @_cdecl("swift_ASTGen_emitParserDiagnostics") public func emitParserDiagnostics( + ctx: BridgedASTContext, diagEnginePtr: UnsafeMutableRawPointer, sourceFilePtr: UnsafeMutablePointer, emitOnlyErrors: CInt, @@ -172,11 +163,18 @@ public func emitParserDiagnostics( ) let diagnosticEngine = BridgedDiagnosticEngine(raw: diagEnginePtr) + let buildConfiguration = CompilerBuildConfiguration( + ctx: ctx, + conditionLoc: + BridgedSourceLoc( + at: AbsolutePosition(utf8Offset: 0), + in: sourceFile.pointee.buffer + ) + ) + for diag in diags { - // Skip over diagnostics within #if, because we don't know whether - // we are in an active region or not. - // FIXME: This heuristic could be improved. - if diag.node.isInIfConfig { + // If the diagnostic is in an unparsed #if region, don't emit it. + if diag.node.isActive(in: buildConfiguration).state == .unparsed { continue } diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp index a3f764bccaf8b..3da1693e47a13 100644 --- a/lib/Parse/ParseDecl.cpp +++ b/lib/Parse/ParseDecl.cpp @@ -267,7 +267,7 @@ void Parser::parseTopLevelItems(SmallVectorImpl &items) { if (parsingOpts.contains(ParsingFlags::ValidateNewParserDiagnostics) && !Context.Diags.hadAnyError()) { auto hadSyntaxError = swift_ASTGen_emitParserDiagnostics( - &Context.Diags, exportedSourceFile, + Context, &Context.Diags, exportedSourceFile, /*emitOnlyErrors=*/true, /*downgradePlaceholderErrorsToWarnings=*/ Context.LangOpts.Playground || @@ -346,7 +346,7 @@ void Parser::parseSourceFileViaASTGen( // If we're supposed to emit diagnostics from the parser, do so now. if (!suppressDiagnostics) { auto hadSyntaxError = swift_ASTGen_emitParserDiagnostics( - &Context.Diags, exportedSourceFile, /*emitOnlyErrors=*/false, + Context, &Context.Diags, exportedSourceFile, /*emitOnlyErrors=*/false, /*downgradePlaceholderErrorsToWarnings=*/langOpts.Playground || langOpts.WarnOnEditorPlaceholder); if (hadSyntaxError && Context.Diags.hadAnyError() && diff --git a/test/ASTGen/if_config.swift b/test/ASTGen/if_config.swift index 1047a23fec9ac..23232d1bd5ca0 100644 --- a/test/ASTGen/if_config.swift +++ b/test/ASTGen/if_config.swift @@ -3,8 +3,8 @@ // REQUIRES: asserts #if NOT_SET -func f { } // FIXME: Error once the parser diagnostics generator knows to - // evaluate the active clause. +func f { } // expected-error{{expected parameter clause in function signature}} + // expected-note@-1{{insert parameter clause}}{{7-8=}}{{8-8=(}}{{8-8=) }} #endif #if compiler(>=10.0) From 0e72c53f02a4bb8de7be5cec9136f9e0176ba632 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Fri, 23 Aug 2024 22:39:40 -0700 Subject: [PATCH 2/3] [SwiftIfConfig] Adopt updated APIs to for build configuration handling Switch to the new `canImport` API that includes TokenSyntax nodes for each import path, so we can provide better source locations. We no longer need to stuff a random source location into `CompilerBuildConfiguration`. Make use of `ConfiguredRegions.isActive(_:)` directly instead of going through the older entrypoint. When parser validation is enabled, we currently can end up with duplicated diagnostics from canImport. This is going to require some requestification to address. --- .../ASTGen+CompilerBuildConfiguration.swift | 2 -- lib/ASTGen/Sources/ASTGen/ASTGen.swift | 2 +- .../ASTGen/CompilerBuildConfiguration.swift | 20 ++++++++++++------- lib/ASTGen/Sources/ASTGen/SourceFile.swift | 14 +++++-------- ..._version_missing_user_module_version.swift | 6 ++++++ 5 files changed, 25 insertions(+), 19 deletions(-) diff --git a/lib/ASTGen/Sources/ASTGen/ASTGen+CompilerBuildConfiguration.swift b/lib/ASTGen/Sources/ASTGen/ASTGen+CompilerBuildConfiguration.swift index 98ec1ebbfcfdd..ac6c126b30d28 100644 --- a/lib/ASTGen/Sources/ASTGen/ASTGen+CompilerBuildConfiguration.swift +++ b/lib/ASTGen/Sources/ASTGen/ASTGen+CompilerBuildConfiguration.swift @@ -25,8 +25,6 @@ extension ASTGenVisitor { /// produced due to the evaluation. func activeClause(in node: IfConfigDeclSyntax) -> IfConfigClauseSyntax? { // Determine the active clause. - var buildConfiguration = self.buildConfiguration - buildConfiguration.conditionLoc = generateSourceLoc(node) let (activeClause, diagnostics) = node.activeClause(in: buildConfiguration) diagnoseAll(diagnostics) diff --git a/lib/ASTGen/Sources/ASTGen/ASTGen.swift b/lib/ASTGen/Sources/ASTGen/ASTGen.swift index 2b5584ed8ee8f..5f1b80f9cd3b0 100644 --- a/lib/ASTGen/Sources/ASTGen/ASTGen.swift +++ b/lib/ASTGen/Sources/ASTGen/ASTGen.swift @@ -98,7 +98,7 @@ struct ASTGenVisitor { self.legacyParse = legacyParser self.buildConfiguration = CompilerBuildConfiguration( ctx: ctx, - conditionLoc: BridgedSourceLoc(at: AbsolutePosition(utf8Offset: 0), in: sourceBuffer) + sourceBuffer: sourceBuffer ) } diff --git a/lib/ASTGen/Sources/ASTGen/CompilerBuildConfiguration.swift b/lib/ASTGen/Sources/ASTGen/CompilerBuildConfiguration.swift index a19d96ff5eeb7..9613f9bf0d38b 100644 --- a/lib/ASTGen/Sources/ASTGen/CompilerBuildConfiguration.swift +++ b/lib/ASTGen/Sources/ASTGen/CompilerBuildConfiguration.swift @@ -20,11 +20,11 @@ import SwiftSyntax /// queries. struct CompilerBuildConfiguration: BuildConfiguration { let ctx: BridgedASTContext - var conditionLoc: BridgedSourceLoc + let sourceBuffer: UnsafeBufferPointer - init(ctx: BridgedASTContext, conditionLoc: BridgedSourceLoc) { + init(ctx: BridgedASTContext, sourceBuffer: UnsafeBufferPointer) { self.ctx = ctx - self.conditionLoc = conditionLoc + self.sourceBuffer = sourceBuffer } func isCustomConditionSet(name: String) throws -> Bool { @@ -48,8 +48,11 @@ struct CompilerBuildConfiguration: BuildConfiguration { } } - func canImport(importPath: [String], version: CanImportVersion) throws -> Bool { - var importPathStr = importPath.joined(separator: ".") + func canImport( + importPath: [(TokenSyntax, String)], + version: CanImportVersion + ) throws -> Bool { + var importPathStr = importPath.map { $0.1 }.joined(separator: ".") var versionComponents: [Int] let cVersionKind: BridgedCanImportVersion @@ -71,7 +74,10 @@ struct CompilerBuildConfiguration: BuildConfiguration { versionComponents.withUnsafeBufferPointer { versionComponentsBuf in ctx.canImport( importPath: bridgedImportPathStr, - location: conditionLoc, + location: BridgedSourceLoc( + at: importPath.first!.0.position, + in: sourceBuffer + ), versionKind: cVersionKind, versionComponents: versionComponentsBuf.baseAddress, numVersionComponents: versionComponentsBuf.count @@ -165,7 +171,7 @@ public func configuredRegions( let sourceFilePtr = sourceFilePtr.bindMemory(to: ExportedSourceFile.self, capacity: 1) let configuration = CompilerBuildConfiguration( ctx: astContext, - conditionLoc: sourceFilePtr.pointee.sourceLoc(at: AbsolutePosition(utf8Offset: 0)) + sourceBuffer: sourceFilePtr.pointee.buffer ) let regions = sourceFilePtr.pointee.syntax.configuredRegions(in: configuration) diff --git a/lib/ASTGen/Sources/ASTGen/SourceFile.swift b/lib/ASTGen/Sources/ASTGen/SourceFile.swift index 4c9dc3e2bbe8b..02d5d4e709197 100644 --- a/lib/ASTGen/Sources/ASTGen/SourceFile.swift +++ b/lib/ASTGen/Sources/ASTGen/SourceFile.swift @@ -158,23 +158,19 @@ public func emitParserDiagnostics( ) { sourceFile in var anyDiags = false - let diags = ParseDiagnosticsGenerator.diagnostics( - for: sourceFile.pointee.syntax - ) + let sourceFileSyntax = sourceFile.pointee.syntax + let diags = ParseDiagnosticsGenerator.diagnostics(for: sourceFileSyntax) let diagnosticEngine = BridgedDiagnosticEngine(raw: diagEnginePtr) let buildConfiguration = CompilerBuildConfiguration( ctx: ctx, - conditionLoc: - BridgedSourceLoc( - at: AbsolutePosition(utf8Offset: 0), - in: sourceFile.pointee.buffer - ) + sourceBuffer: sourceFile.pointee.buffer ) + let configuredRegions = sourceFileSyntax.configuredRegions(in: buildConfiguration) for diag in diags { // If the diagnostic is in an unparsed #if region, don't emit it. - if diag.node.isActive(in: buildConfiguration).state == .unparsed { + if configuredRegions.isActive(diag.node) == .unparsed { continue } diff --git a/test/Parse/ConditionalCompilation/can_import_version_missing_user_module_version.swift b/test/Parse/ConditionalCompilation/can_import_version_missing_user_module_version.swift index 62351eb2d9e29..3803013d53f94 100644 --- a/test/Parse/ConditionalCompilation/can_import_version_missing_user_module_version.swift +++ b/test/Parse/ConditionalCompilation/can_import_version_missing_user_module_version.swift @@ -14,11 +14,17 @@ import NoUserModuleVersion func testCanImportNoUserModuleVersion() { #if canImport(NoUserModuleVersion, _version: 113.331) // expected-warning {{cannot find user version number for Swift module 'NoUserModuleVersion'; version number ignored}} + // NOTE: Duplicate warning because the canImport request happens twice when parser + // validation is enabled. + // expected-warning@-3 *{{cannot find user version number for Swift module 'NoUserModuleVersion'; version number ignored}} let a = 1 // expected-warning {{initialization of immutable value 'a' was never used; consider replacing with assignment to '_' or removing it}} #endif #if canImport(NoUserModuleVersion, _version: 2) // expected-warning {{cannot find user version number for Swift module 'NoUserModuleVersion'; version number ignored}} let b = 1 // expected-warning {{initialization of immutable value 'b' was never used; consider replacing with assignment to '_' or removing it}} + // NOTE: Duplicate warning because the canImport request happens twice when parser + // validation is enabled. + // expected-warning@-4 *{{cannot find user version number for Swift module 'NoUserModuleVersion'; version number ignored}} #endif } From 9b1c9388f23f72d8f46269c388f6b104c0209e1a Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Sat, 24 Aug 2024 08:10:49 -0700 Subject: [PATCH 3/3] Update two more tests for duplicated canImport diagnostics Since these duplicated diagnostics only occur under the ParserValidation testing mode, it won't affect users yet. --- .../can_import_underlying_version_tbd_missing_version.swift | 2 ++ .../can_import_version_ignores_missing_tbd_version.swift | 3 +++ .../can_import_version_missing_user_module_version.swift | 4 ++-- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/test/ClangImporter/can_import_underlying_version_tbd_missing_version.swift b/test/ClangImporter/can_import_underlying_version_tbd_missing_version.swift index af439bdb31c65..5ee46c18ff963 100644 --- a/test/ClangImporter/can_import_underlying_version_tbd_missing_version.swift +++ b/test/ClangImporter/can_import_underlying_version_tbd_missing_version.swift @@ -10,12 +10,14 @@ import Simple func canImportUnderlyingVersion() { #if canImport(Simple, _underlyingVersion: 3.3) // expected-warning {{cannot find user version number for Clang module 'Simple'; version number ignored}} + // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for Clang module 'Simple'; version number ignored}} let a = 1 // expected-warning {{initialization of immutable value 'a' was never used; consider replacing with assignment to '_' or removing it}} #endif } func canImportVersion() { #if canImport(Simple, _version: 3.3) // expected-warning {{cannot find user version number for Clang module 'Simple'; version number ignored}} + // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for Clang module 'Simple'; version number ignored}} let a = 1 // expected-warning {{initialization of immutable value 'a' was never used; consider replacing with assignment to '_' or removing it}} #endif } diff --git a/test/ClangImporter/can_import_version_ignores_missing_tbd_version.swift b/test/ClangImporter/can_import_version_ignores_missing_tbd_version.swift index e68bfa6b00eeb..629c87939524a 100644 --- a/test/ClangImporter/can_import_version_ignores_missing_tbd_version.swift +++ b/test/ClangImporter/can_import_version_ignores_missing_tbd_version.swift @@ -23,14 +23,17 @@ import Simple func canImportUnderlyingVersion() { #if canImport(Simple, _underlyingVersion: 2) // expected-warning {{cannot find user version number for Clang module 'Simple'; version number ignored}} + // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for Clang module 'Simple'; version number ignored}} let a = 1 // expected-warning {{initialization of immutable value 'a' was never used; consider replacing with assignment to '_' or removing it}} #endif #if canImport(Simple, _underlyingVersion: 3) // expected-warning {{cannot find user version number for Clang module 'Simple'; version number ignored}} + // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for Clang module 'Simple'; version number ignored}} let b = 1 // expected-warning {{initialization of immutable value 'b' was never used; consider replacing with assignment to '_' or removing it}} #endif #if canImport(Simple, _underlyingVersion: 4) // expected-warning {{cannot find user version number for Clang module 'Simple'; version number ignored}} + // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for Clang module 'Simple'; version number ignored}} let c = 1 // expected-warning {{initialization of immutable value 'c' was never used; consider replacing with assignment to '_' or removing it}} #endif diff --git a/test/Parse/ConditionalCompilation/can_import_version_missing_user_module_version.swift b/test/Parse/ConditionalCompilation/can_import_version_missing_user_module_version.swift index 3803013d53f94..08fe2c59f8edd 100644 --- a/test/Parse/ConditionalCompilation/can_import_version_missing_user_module_version.swift +++ b/test/Parse/ConditionalCompilation/can_import_version_missing_user_module_version.swift @@ -16,7 +16,7 @@ func testCanImportNoUserModuleVersion() { #if canImport(NoUserModuleVersion, _version: 113.331) // expected-warning {{cannot find user version number for Swift module 'NoUserModuleVersion'; version number ignored}} // NOTE: Duplicate warning because the canImport request happens twice when parser // validation is enabled. - // expected-warning@-3 *{{cannot find user version number for Swift module 'NoUserModuleVersion'; version number ignored}} + // TODO(ParserValidation): expected-warning@-3 *{{cannot find user version number for Swift module 'NoUserModuleVersion'; version number ignored}} let a = 1 // expected-warning {{initialization of immutable value 'a' was never used; consider replacing with assignment to '_' or removing it}} #endif @@ -24,7 +24,7 @@ func testCanImportNoUserModuleVersion() { let b = 1 // expected-warning {{initialization of immutable value 'b' was never used; consider replacing with assignment to '_' or removing it}} // NOTE: Duplicate warning because the canImport request happens twice when parser // validation is enabled. - // expected-warning@-4 *{{cannot find user version number for Swift module 'NoUserModuleVersion'; version number ignored}} + // TODO(ParserValidation): expected-warning@-4 *{{cannot find user version number for Swift module 'NoUserModuleVersion'; version number ignored}} #endif }