From 40862d5384177a54f368cb8933c76db35597288d Mon Sep 17 00:00:00 2001 From: slavek-kucera <53339291+slavek-kucera@users.noreply.github.com> Date: Tue, 3 Jan 2023 14:52:15 +0100 Subject: [PATCH] fix: File extensions are now ignored when `macro_extensions` option is not provided in proc_grps.json --- README.md | 1 + clients/vscode-hlasmplugin/CHANGELOG.md | 1 + parser_library/src/diagnostic.cpp | 26 ++---- parser_library/src/diagnostic.h | 6 +- .../src/workspaces/library_local.cpp | 85 ++++++++++--------- parser_library/src/workspaces/library_local.h | 6 -- .../workspaces/workspace_configuration.cpp | 49 ++--------- .../src/workspaces/workspace_configuration.h | 2 - .../workspace/extension_handling_test.cpp | 42 +++++---- 9 files changed, 92 insertions(+), 126 deletions(-) diff --git a/README.md b/README.md index f6cac91ff..b74a732bc 100644 --- a/README.md +++ b/README.md @@ -219,6 +219,7 @@ The `alwaysRecognize` option in `pgm_conf.json` has been deprecated in favor of `proc_grps.json` can include an optional parameter `macro_extensions` which contains a list of extensions that are used to identify files with macro definitions. The options can be specified both at the top level of the file, which provides the default list for all libraries in all process groups, and at the level of individual library definitions, which override the default from the top level. +If the `macro_extensions` parameter is not provided or is empty the language server ignores file extensions. Warning messages are produced when conflicting names are detected. For example, with the extension `.hlasm`, a user can add the macro `MAC` to his source code even if it is in a file called `MAC.hlasm`. diff --git a/clients/vscode-hlasmplugin/CHANGELOG.md b/clients/vscode-hlasmplugin/CHANGELOG.md index 4c47c7c9c..b369914f5 100644 --- a/clients/vscode-hlasmplugin/CHANGELOG.md +++ b/clients/vscode-hlasmplugin/CHANGELOG.md @@ -23,6 +23,7 @@ #### Changed - Macro label is the preferred go to definition target unless the request is made from the label itself - Library contents are now shared between processor groups +- File extensions are now ignored when `macro_extensions` option is not provided in proc_grps.json ## [1.5.0](https://github.com/eclipse/che-che4z-lsp-for-hlasm/compare/1.4.0...1.5.0) (2022-11-02) diff --git a/parser_library/src/diagnostic.cpp b/parser_library/src/diagnostic.cpp index b38d852b6..650becb70 100644 --- a/parser_library/src/diagnostic.cpp +++ b/parser_library/src/diagnostic.cpp @@ -2660,30 +2660,22 @@ diagnostic_s diagnostic_s::error_L0002( concat("Unable to load library: ", lib_loc.to_presentable(), ". Error: The path does not point to directory.")); } -diagnostic_s diagnostic_s::warning_L0003( - const utils::resource::resource_location& config_loc, const utils::resource::resource_location& lib_loc) -{ - return diagnostic_s(config_loc.get_uri(), - {}, - diagnostic_severity::warning, - "L0003", - concat("Macros from library '", - lib_loc.to_presentable(), - "' were selected by a deprecated mechanism to specify file extensions (alwaysRecognize in pgm_conf.json)."), - {}, - diagnostic_tag::none); -} - diagnostic_s diagnostic_s::warning_L0004(const utils::resource::resource_location& config_loc, const utils::resource::resource_location& lib_loc, - std::string_view macro_name) + std::string_view macro_name, + bool has_extensions) { return diagnostic_s(config_loc.get_uri(), {}, diagnostic_severity::warning, "L0004", - concat( - "Library '", lib_loc.to_presentable(), "' contains multiple definitions of the macro '", macro_name, "'."), + concat("Library '", + lib_loc.to_presentable(), + "' contains conflicting macro definitions (", + macro_name, + "). Consider ", + has_extensions ? std::string_view("changing") : std::string_view("adding"), + " 'macro_extensions' parameter."), {}, diagnostic_tag::none); } diff --git a/parser_library/src/diagnostic.h b/parser_library/src/diagnostic.h index ef17f3cb0..d83ecd30b 100644 --- a/parser_library/src/diagnostic.h +++ b/parser_library/src/diagnostic.h @@ -866,12 +866,12 @@ class diagnostic_s static diagnostic_s error_L0002( const utils::resource::resource_location& config_loc, const utils::resource::resource_location& lib_loc); - static diagnostic_s warning_L0003( - const utils::resource::resource_location& config_loc, const utils::resource::resource_location& lib_loc); + // warning_L0003 - removed static diagnostic_s warning_L0004(const utils::resource::resource_location& config_loc, const utils::resource::resource_location& lib_loc, - std::string_view macro_name); + std::string_view macro_name, + bool has_extensions); static diagnostic_s warning_L0005( const utils::resource::resource_location& config_loc, std::string_view pattern, size_t limit); diff --git a/parser_library/src/workspaces/library_local.cpp b/parser_library/src/workspaces/library_local.cpp index 41edaae72..3657c9cba 100644 --- a/parser_library/src/workspaces/library_local.cpp +++ b/parser_library/src/workspaces/library_local.cpp @@ -16,6 +16,7 @@ #include #include +#include #include "utils/path.h" #include "utils/platform.h" @@ -24,19 +25,12 @@ namespace hlasm_plugin::parser_library::workspaces { namespace { -void adjust_extensions_vector(std::vector& extensions, bool extensions_from_deprecated_source) +void adjust_extensions_vector(std::vector& extensions) { - bool contains_empty = false; for (auto& ext : extensions) - { - if (ext.empty()) - contains_empty = true; - else - { - if (ext[0] != '.') - ext.insert(0, 1, '.'); - } - } + if (!ext.empty() && ext[0] != '.') + ext.insert(0, 1, '.'); + // from longest to shortest, then lexicographically std::sort(extensions.begin(), extensions.end(), [](const std::string& l, const std::string& r) { if (l.size() > r.size()) @@ -45,8 +39,6 @@ void adjust_extensions_vector(std::vector& extensions, bool extensi return false; return l < r; }); - if (extensions_from_deprecated_source && !contains_empty) - extensions.emplace_back(); // alwaysRecognize always implied accepting files without an extension extensions.erase(std::unique(extensions.begin(), extensions.end()), extensions.end()); } } // namespace @@ -59,11 +51,10 @@ library_local::library_local(file_manager& file_manager, , m_lib_loc(std::move(lib_loc)) , m_extensions(std::move(options.extensions)) , m_optional(options.optional_library) - , m_extensions_from_deprecated_source(options.extensions_from_deprecated_source) , m_proc_grps_loc(std::move(proc_grps_loc)) { if (m_extensions.size()) - adjust_extensions_vector(m_extensions, m_extensions_from_deprecated_source); + adjust_extensions_vector(m_extensions); } library_local::library_local(library_local&& l) noexcept @@ -72,7 +63,6 @@ library_local::library_local(library_local&& l) noexcept , m_files_collection(l.m_files_collection.exchange(nullptr)) , m_extensions(std::move(l.m_extensions)) , m_optional(l.m_optional) - , m_extensions_from_deprecated_source(l.m_extensions_from_deprecated_source) , m_proc_grps_loc(std::move(l.m_proc_grps_loc)) {} @@ -138,40 +128,55 @@ library_local::files_collection_t library_local::load_files() break; } - bool extension_removed = false; + size_t conflict_count = 0; + std::string file_name_conflicts; + const auto add_conflict = [&conflict_count, &file_name_conflicts](std::string_view file_name) { + constexpr const size_t max_conflict_count = 3; + if (conflict_count < max_conflict_count) + { + if (conflict_count) + file_name_conflicts.append(", "); + file_name_conflicts.append(file_name); + } + else if (conflict_count == max_conflict_count) + file_name_conflicts.append(" and others"); + ++conflict_count; + }; + for (auto& [file, rl] : files_list) { if (m_extensions.empty()) { - new_files.try_emplace(context::to_upper_copy(file), std::move(rl)); - continue; + // ".hidden" is not an extension ------v + if (auto off = file.find_first_of('.', 1); off != std::string::npos) + file.erase(off); } + else if (auto ext = std::find_if( + m_extensions.begin(), m_extensions.end(), [&f = file](const auto& e) { return f.ends_with(e); }); + ext != m_extensions.end()) - for (const auto& extension : m_extensions) - { - std::string_view filename(file); - - if (filename.size() <= extension.size()) - continue; - - if (filename.substr(filename.size() - extension.size()) != extension) - continue; - filename.remove_suffix(extension.size()); + file.erase(file.size() - ext->size()); + else + continue; - const auto [_, inserted] = - new_files.try_emplace(context::to_upper_copy(std::string(filename)), std::move(rl)); - // TODO: the stored value is a full path, yet we try to interpret it as a relative one later on - if (!inserted) - new_diags.push_back(diagnostic_s::warning_L0004( - m_proc_grps_loc, m_lib_loc, context::to_upper_copy(std::string(filename)))); + context::to_upper(file); - if (extension.size()) - extension_removed = true; - break; + if (auto [it, inserted] = new_files.try_emplace(std::move(file), std::move(rl)); !inserted) + { + // file, rl was not moved + add_conflict(file); + + // keep shortest (i.e. without extension for compatibility) or lexicographically smaller + const std::string_view rl_uri(rl.get_uri()); + const std::string_view old_uri(it->second.get_uri()); + if (std::pair(rl_uri.size(), rl_uri) < std::pair(old_uri.size(), old_uri)) + it->second = std::move(rl); } } - if (extension_removed && m_extensions_from_deprecated_source) - new_diags.push_back(diagnostic_s::warning_L0003(m_proc_grps_loc, m_lib_loc)); + + if (conflict_count > 0) + new_diags.push_back( + diagnostic_s::warning_L0004(m_proc_grps_loc, m_lib_loc, file_name_conflicts, !m_extensions.empty())); m_files_collection.store(new_state); diff --git a/parser_library/src/workspaces/library_local.h b/parser_library/src/workspaces/library_local.h index 00727311a..44f6998e8 100644 --- a/parser_library/src/workspaces/library_local.h +++ b/parser_library/src/workspaces/library_local.h @@ -36,7 +36,6 @@ namespace hlasm_plugin::parser_library::workspaces { struct library_local_options { std::vector extensions; - bool extensions_from_deprecated_source = false; bool optional_library = false; #ifdef __cpp_lib_three_way_comparison @@ -49,10 +48,6 @@ struct library_local_options return true; if (extensions > o.extensions) return false; - if (extensions_from_deprecated_source < o.extensions_from_deprecated_source) - return true; - if (extensions_from_deprecated_source > o.extensions_from_deprecated_source) - return false; if (optional_library < o.optional_library) return true; return false; @@ -118,7 +113,6 @@ class library_local final : public library atomic_files_collection_t m_files_collection; std::vector m_extensions; bool m_optional = false; - bool m_extensions_from_deprecated_source = false; utils::resource::resource_location m_proc_grps_loc; files_collection_t load_files(); diff --git a/parser_library/src/workspaces/workspace_configuration.cpp b/parser_library/src/workspaces/workspace_configuration.cpp index 81d4ef25e..d55de3005 100644 --- a/parser_library/src/workspaces/workspace_configuration.cpp +++ b/parser_library/src/workspaces/workspace_configuration.cpp @@ -71,29 +71,6 @@ utils::resource::resource_location transform_to_resource_location( return rl.lexically_normal(); } -std::vector get_macro_extensions_compatibility_list(std::span always_recognize) -{ - // Extract extension list for compatibility reasons - std::vector macro_extensions_compatibility_list; - for (const auto& wildcard : always_recognize) - { - std::string_view wc(wildcard); - if (const auto ext_pattern = wc.rfind("*."); ext_pattern != std::string_view::npos) - { - wc.remove_prefix(ext_pattern + 1); - macro_extensions_compatibility_list.emplace_back(wc); - } - } - - std::sort(macro_extensions_compatibility_list.begin(), macro_extensions_compatibility_list.end()); - - macro_extensions_compatibility_list.erase( - std::unique(macro_extensions_compatibility_list.begin(), macro_extensions_compatibility_list.end()), - macro_extensions_compatibility_list.end()); - - return macro_extensions_compatibility_list; -} - std::optional substitute_home_directory(std::string p) { if (!p.starts_with("~")) @@ -107,9 +84,8 @@ std::optional substitute_home_directory(std::string p) return utils::path::join(homedir, std::move(p).substr(skip)).string(); } -library_local_options get_library_local_options(const config::library& lib, - std::span fallback_macro_extensions, - std::span always_recognize) +library_local_options get_library_local_options( + const config::library& lib, std::span fallback_macro_extensions) { library_local_options opts; @@ -118,12 +94,6 @@ library_local_options get_library_local_options(const config::library& lib, opts.extensions = lib.macro_extensions; else if (!fallback_macro_extensions.empty()) opts.extensions = std::vector(fallback_macro_extensions.begin(), fallback_macro_extensions.end()); - else if (auto macro_extensions_compatibility_list = get_macro_extensions_compatibility_list(always_recognize); - !macro_extensions_compatibility_list.empty()) - { - opts.extensions = macro_extensions_compatibility_list; - opts.extensions_from_deprecated_source = true; - } return opts; } @@ -292,7 +262,6 @@ std::shared_ptr workspace_configuration::get_local_library( void workspace_configuration::process_processor_group(const config::processor_group& pg, std::span fallback_macro_extensions, - std::span always_recognize, const utils::resource::resource_location& alternative_root, std::vector& diags) { @@ -312,7 +281,7 @@ void workspace_configuration::process_processor_group(const config::processor_gr continue; } - auto lib_local_opts = get_library_local_options(lib, fallback_macro_extensions, always_recognize); + auto lib_local_opts = get_library_local_options(lib, fallback_macro_extensions); auto rl = transform_to_resource_location(*lib_path, root); rl.join(""); // Ensure that this is a directory @@ -332,7 +301,6 @@ void workspace_configuration::process_processor_group(const config::processor_gr void workspace_configuration::process_processor_group_and_cleanup_libraries( std::span pgs, std::span fallback_macro_extensions, - std::span always_recognize, const utils::resource::resource_location& alternative_root, std::vector& diags) { @@ -340,7 +308,7 @@ void workspace_configuration::process_processor_group_and_cleanup_libraries( l.second = false; // mark for (const auto& pg : pgs) - process_processor_group(pg, fallback_macro_extensions, always_recognize, alternative_root, diags); + process_processor_group(pg, fallback_macro_extensions, alternative_root, diags); std::erase_if(m_libraries, [](const auto& kv) { return !kv.second.second; }); // sweep } @@ -396,11 +364,8 @@ parse_config_file_result workspace_configuration::load_and_process_config(std::v config::pgm_conf pgm_config; const auto pgm_conf_loaded = load_pgm_config(pgm_config, utilized_settings_values, diags); - process_processor_group_and_cleanup_libraries(proc_groups.pgroups, - proc_groups.macro_extensions, - pgm_config.always_recognize, - utils::resource::resource_location(), - diags); + process_processor_group_and_cleanup_libraries( + proc_groups.pgroups, proc_groups.macro_extensions, utils::resource::resource_location(), diags); if (pgm_conf_loaded != parse_config_file_result::parsed) { @@ -546,7 +511,7 @@ parse_config_file_result workspace_configuration::parse_b4g_config_file( } process_processor_group_and_cleanup_libraries( - m_proc_grps_source.pgroups, m_proc_grps_source.macro_extensions, {}, alternative_root, conf.diags); + m_proc_grps_source.pgroups, m_proc_grps_source.macro_extensions, alternative_root, conf.diags); for (const auto& [name, details] : conf.config.value().files) { diff --git a/parser_library/src/workspaces/workspace_configuration.h b/parser_library/src/workspaces/workspace_configuration.h index 1c0606449..c10d512c6 100644 --- a/parser_library/src/workspaces/workspace_configuration.h +++ b/parser_library/src/workspaces/workspace_configuration.h @@ -227,13 +227,11 @@ class workspace_configuration void process_processor_group(const config::processor_group& pg, std::span fallback_macro_extensions, - std::span always_recognize, const utils::resource::resource_location& alternative_root, std::vector& diags); void process_processor_group_and_cleanup_libraries(std::span pgs, std::span fallback_macro_extensions, - std::span always_recognize, const utils::resource::resource_location& alternative_root, std::vector& diags); diff --git a/parser_library/test/workspace/extension_handling_test.cpp b/parser_library/test/workspace/extension_handling_test.cpp index d7763dbe9..39a8f2614 100644 --- a/parser_library/test/workspace/extension_handling_test.cpp +++ b/parser_library/test/workspace/extension_handling_test.cpp @@ -26,8 +26,6 @@ using namespace hlasm_plugin::utils::resource; namespace { const auto lib_loc = hlasm_plugin::utils::platform::is_windows() ? resource_location("lib\\") : resource_location("lib/"); -const auto lib2_loc = - hlasm_plugin::utils::platform::is_windows() ? resource_location("lib2\\") : resource_location("lib2/"); } // namespace class file_manager_extension_mock : public file_manager_impl @@ -57,15 +55,15 @@ TEST(extension_handling_test, extension_removal) EXPECT_FALSE(lib3.has_file("MAC")); // test multiple extensions - library_local lib4(file_mngr, lib2_loc, { { ".hlasm", ".asm" } }, empty_loc); + library_local lib4(file_mngr, lib_loc, { { ".hlasm", ".asm" } }, empty_loc); EXPECT_TRUE(lib4.has_file("MAC")); // test no extensions - library_local lib5(file_mngr, lib2_loc, { {} }, empty_loc); - EXPECT_FALSE(lib5.has_file("MAC")); + library_local lib5(file_mngr, lib_loc, { {} }, empty_loc); + EXPECT_TRUE(lib5.has_file("MAC")); - // test no extensions - library_local lib6(file_mngr, lib2_loc, { { "" } }, empty_loc); + // test empty extension + library_local lib6(file_mngr, lib_loc, { { "" } }, empty_loc); EXPECT_FALSE(lib6.has_file("MAC")); // tolerate missing dot @@ -77,12 +75,12 @@ TEST(extension_handling_test, legacy_extension_selection) { file_manager_extension_mock file_mngr; resource_location empty_loc; - library_local lib(file_mngr, lib_loc, { { ".hlasm" }, true }, empty_loc); + library_local lib(file_mngr, lib_loc, { { ".hlasm" } }, empty_loc); EXPECT_TRUE(lib.has_file("MAC")); - std::vector diags; + std::vector diags; lib.copy_diagnostics(diags); - EXPECT_TRUE(std::any_of(diags.begin(), diags.end(), [](const auto& d) { return d.code == "L0003"; })); + EXPECT_TRUE(diags.empty()); } class file_manager_extension_mock2 : public file_manager_impl @@ -102,7 +100,7 @@ TEST(extension_handling_test, multiple_macro_definitions) library_local lib(file_mngr, lib_loc, { { ".hlasm", "" } }, empty_loc); EXPECT_TRUE(lib.has_file("MAC")); - std::vector diags; + std::vector diags; lib.copy_diagnostics(diags); EXPECT_TRUE(std::any_of(diags.begin(), diags.end(), [](const auto& d) { return d.code == "L0004"; })); } @@ -114,11 +112,23 @@ TEST(extension_handling_test, no_multiple_macro_definitions) library_local lib(file_mngr, lib_loc, { { ".hlasm" } }, empty_loc); EXPECT_TRUE(lib.has_file("MAC")); - std::vector diags; + std::vector diags; lib.copy_diagnostics(diags); EXPECT_TRUE(std::none_of(diags.begin(), diags.end(), [](const auto& d) { return d.code == "L0004"; })); } +TEST(extension_handling_test, multiple_macros_extensions_not_provided) +{ + file_manager_extension_mock2 file_mngr; + resource_location empty_loc; + library_local lib(file_mngr, lib_loc, {}, empty_loc); + + EXPECT_TRUE(lib.has_file("MAC")); + std::vector diags; + lib.copy_diagnostics(diags); + EXPECT_EQ(std::count_if(diags.begin(), diags.end(), [](const auto& d) { return d.code == "L0004"; }), 1); +} + class file_manager_extension_mock_no_ext : public file_manager_impl { list_directory_result list_directory_files(const resource_location&) const override @@ -132,10 +142,10 @@ TEST(extension_handling_test, legacy_extension_selection_file_without_ext) { file_manager_extension_mock_no_ext file_mngr; resource_location empty_loc; - library_local lib(file_mngr, lib_loc, { { ".hlasm" }, true }, empty_loc); + library_local lib(file_mngr, lib_loc, { { ".hlasm" } }, empty_loc); - EXPECT_TRUE(lib.has_file("MAC")); - std::vector diags; + EXPECT_FALSE(lib.has_file("MAC")); + std::vector diags; lib.copy_diagnostics(diags); - EXPECT_TRUE(std::none_of(diags.begin(), diags.end(), [](const auto& d) { return d.code == "L0003"; })); + EXPECT_TRUE(diags.empty()); }