Просмотр исходного кода

Diagnose incorrect file extensions. (#3409)

Note this is building on #3408
Jon Ross-Perkins 2 лет назад
Родитель
Сommit
9d369db8ea

+ 73 - 35
toolchain/check/check.cpp

@@ -9,6 +9,7 @@
 #include "toolchain/base/value_store.h"
 #include "toolchain/check/context.h"
 #include "toolchain/diagnostics/diagnostic_emitter.h"
+#include "toolchain/lex/token_kind.h"
 #include "toolchain/parse/tree.h"
 #include "toolchain/parse/tree_node_location_translator.h"
 #include "toolchain/sem_ir/file.h"
@@ -239,25 +240,14 @@ static auto TrackImport(
   }
 }
 
-auto CheckParseTrees(const SemIR::File& builtin_ir,
-                     llvm::MutableArrayRef<Unit> units,
-                     llvm::raw_ostream* vlog_stream) -> void {
-  // Prepare diagnostic emitters in case we run into issues during package
-  // checking.
-  //
-  // UnitInfo is big due to its SmallVectors, so we default to 0 on the stack.
-  llvm::SmallVector<UnitInfo, 0> unit_infos;
-  unit_infos.reserve(units.size());
-  for (auto& unit : units) {
-    unit_infos.emplace_back(unit);
-  }
-
-  // Create a map of APIs which might be imported.
+// Builds a map of `api` files which might be imported. Also diagnoses issues
+// related to the packaging because the strings are loaded as part of getting
+// the ImportKey (which we then do for `impl` files too).
+static auto BuildApiMapAndDiagnosePackaging(
+    llvm::SmallVector<UnitInfo, 0>& unit_infos)
+    -> llvm::DenseMap<ImportKey, UnitInfo*> {
   llvm::DenseMap<ImportKey, UnitInfo*> api_map;
   for (auto& unit_info : unit_infos) {
-    // TODO: It may be good to validate filenames here, but that would have use
-    // put .impl.carbon on almost all tests (which are in `Main//default`). We
-    // should probably get leads direction on filenames before enforcing.
     const auto& packaging = unit_info.unit->parse_tree->packaging_directive();
     // An import key formed from the `package` or `library` directive. Or, for
     // Main//default, a placeholder key.
@@ -279,29 +269,77 @@ auto CheckParseTrees(const SemIR::File& builtin_ir,
       continue;
     }
 
-    if (packaging && packaging->api_or_impl == Parse::Tree::ApiOrImpl::Impl) {
-      continue;
+    bool is_impl =
+        packaging && packaging->api_or_impl == Parse::Tree::ApiOrImpl::Impl;
+
+    // Add to the `api` map and diagnose duplicates. This occurs before the
+    // file extension check because we might emit both diagnostics in situation
+    // where the user forgets (or has syntax errors with) a package line
+    // multiple times.
+    if (!is_impl) {
+      auto [entry, success] = api_map.insert({import_key, &unit_info});
+      if (!success) {
+        llvm::StringRef prev_filename = entry->second->unit->tokens->filename();
+        if (packaging) {
+          CARBON_DIAGNOSTIC(DuplicateLibraryApi, Error,
+                            "Library's API previously provided by `{0}`.",
+                            llvm::StringRef);
+          unit_info.emitter.Emit(packaging->names.node, DuplicateLibraryApi,
+                                 prev_filename);
+        } else {
+          CARBON_DIAGNOSTIC(DuplicateMainApi, Error,
+                            "Main//default previously provided by `{0}`.",
+                            llvm::StringRef);
+          // Use the invalid node because there's no node to associate with.
+          unit_info.emitter.Emit(Parse::Node::Invalid, DuplicateMainApi,
+                                 prev_filename);
+        }
+      }
     }
 
-    auto [entry, success] = api_map.insert({import_key, &unit_info});
-    if (!success) {
-      llvm::StringRef prev_filename = entry->second->unit->tokens->filename();
-      if (packaging) {
-        CARBON_DIAGNOSTIC(DuplicateLibraryApi, Error,
-                          "Library's API previously provided by `{0}`.",
-                          llvm::StringRef);
-        unit_info.emitter.Emit(packaging->names.node, DuplicateLibraryApi,
-                               prev_filename);
-      } else {
-        CARBON_DIAGNOSTIC(DuplicateMainApi, Error,
-                          "Main//default previously provided by `{0}`.",
-                          llvm::StringRef);
-        // Use the invalid node because there's no node to associate with.
-        unit_info.emitter.Emit(Parse::Node::Invalid, DuplicateMainApi,
-                               prev_filename);
+    // Validate file extensions. Note imports rely the packaging directive, not
+    // the extension.
+    auto filename = unit_info.unit->tokens->filename();
+    static constexpr llvm::StringLiteral ApiExt = ".carbon";
+    static constexpr llvm::StringLiteral ImplExt = ".impl.carbon";
+    bool is_api_with_impl_ext = !is_impl && filename.ends_with(ImplExt);
+    auto want_ext = is_impl ? ImplExt : ApiExt;
+    if (is_api_with_impl_ext || !filename.ends_with(want_ext)) {
+      CARBON_DIAGNOSTIC(IncorrectExtension, Error,
+                        "File extension of `{0}` required for `{1}`.",
+                        llvm::StringLiteral, Lex::TokenKind);
+      auto diag = unit_info.emitter.Build(
+          packaging ? packaging->names.node : Parse::Node::Invalid,
+          IncorrectExtension, want_ext,
+          is_impl ? Lex::TokenKind::Impl : Lex::TokenKind::Api);
+      if (is_api_with_impl_ext) {
+        CARBON_DIAGNOSTIC(IncorrectExtensionImplNote, Note,
+                          "File extension of `{0}` only allowed for `{1}`.",
+                          llvm::StringLiteral, Lex::TokenKind);
+        diag.Note(Parse::Node::Invalid, IncorrectExtensionImplNote, ImplExt,
+                  Lex::TokenKind::Impl);
       }
+      diag.Emit();
     }
   }
+  return api_map;
+}
+
+auto CheckParseTrees(const SemIR::File& builtin_ir,
+                     llvm::MutableArrayRef<Unit> units,
+                     llvm::raw_ostream* vlog_stream) -> void {
+  // Prepare diagnostic emitters in case we run into issues during package
+  // checking.
+  //
+  // UnitInfo is big due to its SmallVectors, so we default to 0 on the stack.
+  llvm::SmallVector<UnitInfo, 0> unit_infos;
+  unit_infos.reserve(units.size());
+  for (auto& unit : units) {
+    unit_infos.emplace_back(unit);
+  }
+
+  llvm::DenseMap<ImportKey, UnitInfo*> api_map =
+      BuildApiMapAndDiagnosePackaging(unit_infos);
 
   // Mark down imports for all files.
   llvm::SmallVector<UnitInfo*> ready_to_check;

+ 9 - 9
toolchain/check/testdata/packages/fail_api_not_found.carbon

@@ -4,30 +4,30 @@
 //
 // AUTOUPDATE
 
-// --- no_api.carbon
+// --- no_api.impl.carbon
 
-// CHECK:STDERR: no_api.carbon:[[@LINE+3]]:1: ERROR: Corresponding API not found.
+// CHECK:STDERR: no_api.impl.carbon:[[@LINE+3]]:1: ERROR: Corresponding API not found.
 // CHECK:STDERR: package Foo impl;
 // CHECK:STDERR: ^
 package Foo impl;
 
-// --- no_api_lib.carbon
+// --- no_api_lib.impl.carbon
 
-// CHECK:STDERR: no_api_lib.carbon:[[@LINE+3]]:1: ERROR: Corresponding API not found.
+// CHECK:STDERR: no_api_lib.impl.carbon:[[@LINE+3]]:1: ERROR: Corresponding API not found.
 // CHECK:STDERR: package Foo library "Bar" impl;
 // CHECK:STDERR: ^
 package Foo library "Bar" impl;
 
-// --- no_api_main_lib.carbon
+// --- no_api_main_lib.impl.carbon
 
-// CHECK:STDERR: no_api_main_lib.carbon:[[@LINE+3]]:1: ERROR: Corresponding API not found.
+// CHECK:STDERR: no_api_main_lib.impl.carbon:[[@LINE+3]]:1: ERROR: Corresponding API not found.
 // CHECK:STDERR: library "Bar" impl;
 // CHECK:STDERR: ^
 library "Bar" impl;
 
-// CHECK:STDOUT: file "no_api.carbon" {
+// CHECK:STDOUT: file "no_api.impl.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "no_api_lib.carbon" {
+// CHECK:STDOUT: file "no_api_lib.impl.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "no_api_main_lib.carbon" {
+// CHECK:STDOUT: file "no_api_main_lib.impl.carbon" {
 // CHECK:STDOUT: }

+ 3 - 3
toolchain/check/testdata/packages/fail_cycle.carbon

@@ -31,9 +31,9 @@ package C api;
 // CHECK:STDERR: ^
 import A;
 
-// --- c_impl.carbon
+// --- c.impl.carbon
 
-// CHECK:STDERR: c_impl.carbon:[[@LINE+3]]:1: ERROR: Import cannot be used due to a cycle. Cycle must be fixed to import.
+// CHECK:STDERR: c.impl.carbon:[[@LINE+3]]:1: ERROR: Import cannot be used due to a cycle. Cycle must be fixed to import.
 // CHECK:STDERR: package C impl;
 // CHECK:STDERR: ^
 package C impl;
@@ -53,7 +53,7 @@ import B;
 // CHECK:STDOUT: }
 // CHECK:STDOUT: file "c.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "c_impl.carbon" {
+// CHECK:STDOUT: file "c.impl.carbon" {
 // CHECK:STDOUT: }
 // CHECK:STDOUT: file "cycle_child.carbon" {
 // CHECK:STDOUT: }

+ 92 - 0
toolchain/check/testdata/packages/fail_extension.carbon

@@ -0,0 +1,92 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// AUTOUPDATE
+// CHECK:STDERR: main.incorrect: ERROR: File extension of `.carbon` required for `api`.
+// CHECK:STDERR: main_redundant_with_swapped_ext.impl.carbon: ERROR: Main//default previously provided by `main.incorrect`.
+// CHECK:STDERR: main_redundant_with_swapped_ext.impl.carbon: ERROR: File extension of `.carbon` required for `api`.
+// CHECK:STDERR: main_redundant_with_swapped_ext.impl.carbon: File extension of `.impl.carbon` only allowed for `impl`.
+
+// --- main.incorrect
+
+// --- main_redundant_with_swapped_ext.impl.carbon
+
+// --- main_lib.incorrect
+
+// CHECK:STDERR: main_lib.incorrect:[[@LINE+3]]:1: ERROR: File extension of `.carbon` required for `api`.
+// CHECK:STDERR: library "lib" api;
+// CHECK:STDERR: ^
+library "lib" api;
+
+// --- main_lib_impl.incorrect
+
+// CHECK:STDERR: main_lib_impl.incorrect:[[@LINE+3]]:1: ERROR: File extension of `.impl.carbon` required for `impl`.
+// CHECK:STDERR: library "lib" impl;
+// CHECK:STDERR: ^
+library "lib" impl;
+
+// --- package.incorrect
+
+// CHECK:STDERR: package.incorrect:[[@LINE+3]]:1: ERROR: File extension of `.carbon` required for `api`.
+// CHECK:STDERR: package Package api;
+// CHECK:STDERR: ^
+package Package api;
+
+// --- package_impl.incorrect
+
+// CHECK:STDERR: package_impl.incorrect:[[@LINE+3]]:1: ERROR: File extension of `.impl.carbon` required for `impl`.
+// CHECK:STDERR: package Package impl;
+// CHECK:STDERR: ^
+package Package impl;
+
+// --- package_lib.incorrect
+
+// CHECK:STDERR: package_lib.incorrect:[[@LINE+3]]:1: ERROR: File extension of `.carbon` required for `api`.
+// CHECK:STDERR: package Package library "lib" api;
+// CHECK:STDERR: ^
+package Package library "lib" api;
+
+// --- package_lib_impl.incorrect
+
+// CHECK:STDERR: package_lib_impl.incorrect:[[@LINE+3]]:1: ERROR: File extension of `.impl.carbon` required for `impl`.
+// CHECK:STDERR: package Package library "lib" impl;
+// CHECK:STDERR: ^
+package Package library "lib" impl;
+
+// --- swapped_ext.impl.carbon
+
+// CHECK:STDERR: swapped_ext.impl.carbon:[[@LINE+4]]:1: ERROR: File extension of `.carbon` required for `api`.
+// CHECK:STDERR: package SwappedExt api;
+// CHECK:STDERR: ^
+// CHECK:STDERR: swapped_ext.impl.carbon: File extension of `.impl.carbon` only allowed for `impl`.
+package SwappedExt api;
+
+// --- swapped_ext.carbon
+
+// CHECK:STDERR: swapped_ext.carbon:[[@LINE+3]]:1: ERROR: File extension of `.impl.carbon` required for `impl`.
+// CHECK:STDERR: package SwappedExt impl;
+// CHECK:STDERR: ^
+package SwappedExt impl;
+
+
+// CHECK:STDOUT: file "main.incorrect" {
+// CHECK:STDOUT: }
+// CHECK:STDOUT: file "main_redundant_with_swapped_ext.impl.carbon" {
+// CHECK:STDOUT: }
+// CHECK:STDOUT: file "main_lib.incorrect" {
+// CHECK:STDOUT: }
+// CHECK:STDOUT: file "main_lib_impl.incorrect" {
+// CHECK:STDOUT: }
+// CHECK:STDOUT: file "package.incorrect" {
+// CHECK:STDOUT: }
+// CHECK:STDOUT: file "package_impl.incorrect" {
+// CHECK:STDOUT: }
+// CHECK:STDOUT: file "package_lib.incorrect" {
+// CHECK:STDOUT: }
+// CHECK:STDOUT: file "package_lib_impl.incorrect" {
+// CHECK:STDOUT: }
+// CHECK:STDOUT: file "swapped_ext.impl.carbon" {
+// CHECK:STDOUT: }
+// CHECK:STDOUT: file "swapped_ext.carbon" {
+// CHECK:STDOUT: }

+ 3 - 3
toolchain/check/testdata/packages/fail_import_default.carbon

@@ -13,11 +13,11 @@ package A api;
 // CHECK:STDERR: ^
 import library default;
 
-// --- default_impl.carbon
+// --- default.impl.carbon
 
 package A impl;
 
-// CHECK:STDERR: default_impl.carbon:[[@LINE+3]]:1: ERROR: Explicit import of `api` from `impl` file is redundant with implicit import.
+// CHECK:STDERR: default.impl.carbon:[[@LINE+3]]:1: ERROR: Explicit import of `api` from `impl` file is redundant with implicit import.
 // CHECK:STDERR: import library default;
 // CHECK:STDERR: ^
 import library default;
@@ -40,7 +40,7 @@ import library default;
 
 // CHECK:STDOUT: file "default_api.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "default_impl.carbon" {
+// CHECK:STDOUT: file "default.impl.carbon" {
 // CHECK:STDOUT: }
 // CHECK:STDOUT: file "main_import_default.carbon" {
 // CHECK:STDOUT: }

+ 6 - 6
toolchain/check/testdata/packages/fail_import_invalid.carbon

@@ -52,11 +52,11 @@ import library "lib";
 
 package Implicit api;
 
-// --- implicit_impl.carbon
+// --- implicit.impl.carbon
 
 package Implicit impl;
 
-// CHECK:STDERR: implicit_impl.carbon:[[@LINE+3]]:1: ERROR: Explicit import of `api` from `impl` file is redundant with implicit import.
+// CHECK:STDERR: implicit.impl.carbon:[[@LINE+3]]:1: ERROR: Explicit import of `api` from `impl` file is redundant with implicit import.
 // CHECK:STDERR: import Implicit;
 // CHECK:STDERR: ^
 import Implicit;
@@ -65,11 +65,11 @@ import Implicit;
 
 package Implicit library "lib" api;
 
-// --- implicit_lib_impl.carbon
+// --- implicit_lib.impl.carbon
 
 package Implicit library "lib" impl;
 
-// CHECK:STDERR: implicit_lib_impl.carbon:[[@LINE+3]]:1: ERROR: Explicit import of `api` from `impl` file is redundant with implicit import.
+// CHECK:STDERR: implicit_lib.impl.carbon:[[@LINE+3]]:1: ERROR: Explicit import of `api` from `impl` file is redundant with implicit import.
 // CHECK:STDERR: import Implicit library "lib";
 // CHECK:STDERR: ^
 import Implicit library "lib";
@@ -92,11 +92,11 @@ import ImportNotFound;
 // CHECK:STDOUT: }
 // CHECK:STDOUT: file "implicit_api.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "implicit_impl.carbon" {
+// CHECK:STDOUT: file "implicit.impl.carbon" {
 // CHECK:STDOUT: }
 // CHECK:STDOUT: file "implicit_lib_api.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "implicit_lib_impl.carbon" {
+// CHECK:STDOUT: file "implicit_lib.impl.carbon" {
 // CHECK:STDOUT: }
 // CHECK:STDOUT: file "not_found.carbon" {
 // CHECK:STDOUT: }

+ 12 - 12
toolchain/check/testdata/packages/implicit_imports.carbon

@@ -12,24 +12,24 @@ package ApiOnly api;
 
 package ApiOnly library "lib" api;
 
-// --- with_impl_api.carbon
+// --- with_impl.carbon
 
 package WithImpl api;
 
-// --- with_impl_impl.carbon
+// --- with_impl.impl.carbon
 
 package WithImpl impl;
 
-// --- with_impl_impl_extra.carbon
+// --- with_impl_extra.impl.carbon
 
 // Multiple impls are allowed.
 package WithImpl impl;
 
-// --- with_impl_lib_api.carbon
+// --- with_impl_lib.carbon
 
 package WithImpl library "lib" api;
 
-// --- with_impl_lib_impl.carbon
+// --- with_impl_lib.impl.carbon
 
 package WithImpl library "lib" impl;
 
@@ -39,7 +39,7 @@ package WithImpl library "lib" impl;
 
 library "lib" api;
 
-// --- main_lib_impl.carbon
+// --- main_lib.impl.carbon
 
 library "lib" impl;
 
@@ -47,19 +47,19 @@ library "lib" impl;
 // CHECK:STDOUT: }
 // CHECK:STDOUT: file "api_only_lib.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "with_impl_api.carbon" {
+// CHECK:STDOUT: file "with_impl.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "with_impl_impl.carbon" {
+// CHECK:STDOUT: file "with_impl.impl.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "with_impl_impl_extra.carbon" {
+// CHECK:STDOUT: file "with_impl_extra.impl.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "with_impl_lib_api.carbon" {
+// CHECK:STDOUT: file "with_impl_lib.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "with_impl_lib_impl.carbon" {
+// CHECK:STDOUT: file "with_impl_lib.impl.carbon" {
 // CHECK:STDOUT: }
 // CHECK:STDOUT: file "main.carbon" {
 // CHECK:STDOUT: }
 // CHECK:STDOUT: file "main_lib.carbon" {
 // CHECK:STDOUT: }
-// CHECK:STDOUT: file "main_lib_impl.carbon" {
+// CHECK:STDOUT: file "main_lib.impl.carbon" {
 // CHECK:STDOUT: }

+ 2 - 0
toolchain/diagnostics/diagnostic_kind.def

@@ -117,6 +117,8 @@ CARBON_DIAGNOSTIC_KIND(ParamsRequiredByIntroducer)
 CARBON_DIAGNOSTIC_KIND(SemanticsTodo)
 
 // Package/import checking diagnostics.
+CARBON_DIAGNOSTIC_KIND(IncorrectExtension)
+CARBON_DIAGNOSTIC_KIND(IncorrectExtensionImplNote)
 CARBON_DIAGNOSTIC_KIND(DuplicateLibraryApi)
 CARBON_DIAGNOSTIC_KIND(DuplicateMainApi)
 CARBON_DIAGNOSTIC_KIND(LibraryApiNotFound)