From d40009511297e84f3c0f1e7ce665f06b85479745 Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Mon, 18 Jul 2022 18:34:08 +0800 Subject: [PATCH 1/2] Decouple warning detection from program instances --- spec/compiler/semantic/warnings_spec.cr | 12 +- spec/spec_helper.cr | 2 +- src/compiler/crystal/command.cr | 34 +++--- src/compiler/crystal/compiler.cr | 17 +-- src/compiler/crystal/macros/methods.cr | 4 +- .../crystal/semantic/abstract_def_checker.cr | 2 +- src/compiler/crystal/semantic/ast.cr | 9 -- .../crystal/semantic/top_level_visitor.cr | 4 +- src/compiler/crystal/semantic/warnings.cr | 83 ++----------- src/compiler/crystal/warnings.cr | 110 ++++++++++++++++++ 10 files changed, 148 insertions(+), 129 deletions(-) create mode 100644 src/compiler/crystal/warnings.cr diff --git a/spec/compiler/semantic/warnings_spec.cr b/spec/compiler/semantic/warnings_spec.cr index 61497b95d573..fef88e44e9b1 100644 --- a/spec/compiler/semantic/warnings_spec.cr +++ b/spec/compiler/semantic/warnings_spec.cr @@ -227,12 +227,12 @@ describe "Semantic: warnings" do CR compiler = create_spec_compiler - compiler.warnings = Warnings::All - compiler.warnings_exclude << Crystal.normalize_path "lib" + compiler.warnings.level = :all + compiler.warnings.exclude_lib_path = true compiler.prelude = "empty" result = compiler.compile Compiler::Source.new(main_filename, File.read(main_filename)), output_filename - result.program.warning_failures.size.should eq(1) + compiler.warnings.infos.size.should eq(1) end end end @@ -396,12 +396,12 @@ describe "Semantic: warnings" do ) compiler = create_spec_compiler - compiler.warnings = Warnings::All - compiler.warnings_exclude << Crystal.normalize_path "lib" + compiler.warnings.level = :all + compiler.warnings.exclude_lib_path = true compiler.prelude = "empty" result = compiler.compile Compiler::Source.new(main_filename, File.read(main_filename)), output_filename - result.program.warning_failures.size.should eq(1) + compiler.warnings.infos.size.should eq(1) end end end diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index 7052f302c7d2..026fab0a9ab7 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -117,7 +117,7 @@ def assert_no_errors(*args, **opts) end def warnings_result(code) - semantic(code).program.warning_failures + semantic(code).program.warnings.infos end def assert_warning(code, message, *, file = __FILE__, line = __LINE__) diff --git a/src/compiler/crystal/command.cr b/src/compiler/crystal/command.cr index 46af92568945..bb7f76464443 100644 --- a/src/compiler/crystal/command.cr +++ b/src/compiler/crystal/command.cr @@ -592,30 +592,26 @@ class Crystal::Command end private def setup_compiler_warning_options(opts, compiler) - compiler.warnings_exclude << Crystal.normalize_path "lib" - warnings_exclude_default = true - opts.on("--warnings all|none", "Which warnings detect. (default: all)") do |w| - compiler.warnings = case w - when "all" - Crystal::Warnings::All - when "none" - Crystal::Warnings::None - else - error "--warnings should be all, or none" - raise "unreachable" - end + opts.on("--warnings all|none", "Which warnings to detect. (default: all)") do |w| + compiler.warnings.level = case w + when "all" + Crystal::WarningLevel::All + when "none" + Crystal::WarningLevel::None + else + error "--warnings should be all, or none" + raise "unreachable" + end end opts.on("--error-on-warnings", "Treat warnings as errors.") do |w| - compiler.error_on_warnings = true + compiler.warnings.error_on_warnings = true end opts.on("--exclude-warnings ", "Exclude warnings from path (default: lib)") do |f| - if warnings_exclude_default - # if an --exclude-warnings is used explicitly, then we remove the default value - compiler.warnings_exclude.clear - warnings_exclude_default = false - end - compiler.warnings_exclude << Crystal.normalize_path f + compiler.warnings.exclude_lib_path = false + compiler.warnings.exclude_path(f) end + + compiler.warnings.exclude_lib_path = true end private def validate_emit_values(values) diff --git a/src/compiler/crystal/compiler.cr b/src/compiler/crystal/compiler.cr index bff7bb4e3dd9..d00753354e81 100644 --- a/src/compiler/crystal/compiler.cr +++ b/src/compiler/crystal/compiler.cr @@ -15,11 +15,6 @@ module Crystal Default = LineNumbers end - enum Warnings - All - None - end - # Main interface to the compiler. # # A Compiler parses source code, type checks it and @@ -109,14 +104,8 @@ module Crystal # and can later be used to generate API docs. property? wants_doc = false - # Which kind of warnings wants to be detected. - property warnings : Warnings = Warnings::All - - # Paths to ignore for warnings detection. - property warnings_exclude : Array(String) = [] of String - - # If `true` compiler will error if warnings are found. - property error_on_warnings : Bool = false + # Warning settings and all detected warnings. + property warnings = WarningCollection.new @[Flags] enum EmitTarget @@ -218,8 +207,6 @@ module Crystal program.show_error_trace = show_error_trace? program.progress_tracker = @progress_tracker program.warnings = @warnings - program.warnings_exclude = @warnings_exclude.map { |p| File.expand_path p } - program.error_on_warnings = @error_on_warnings program end diff --git a/src/compiler/crystal/macros/methods.cr b/src/compiler/crystal/macros/methods.cr index 4058f8590080..83cd8f910d63 100644 --- a/src/compiler/crystal/macros/methods.cr +++ b/src/compiler/crystal/macros/methods.cr @@ -37,7 +37,7 @@ module Crystal filename end - delegate report_warning_at, to: @program + delegate warnings, to: @program def interpret_top_level_call(node) interpret_top_level_call?(node) || @@ -2199,7 +2199,7 @@ module Crystal ArrayLiteral.map(@names) { |name| MacroId.new(name) } end when "global" - interpreter.report_warning_at(name_loc, "Deprecated Path#global. Use `#global?` instead") + interpreter.warnings.add_warning_at(name_loc, "Deprecated Path#global. Use `#global?` instead") interpret_check_args { BoolLiteral.new(@global) } when "global?" interpret_check_args { BoolLiteral.new(@global) } diff --git a/src/compiler/crystal/semantic/abstract_def_checker.cr b/src/compiler/crystal/semantic/abstract_def_checker.cr index 09705a8f9599..8f11c5434da5 100644 --- a/src/compiler/crystal/semantic/abstract_def_checker.cr +++ b/src/compiler/crystal/semantic/abstract_def_checker.cr @@ -375,7 +375,7 @@ class Crystal::AbstractDefChecker impl_param = impl_method.args[i] base_param = base_method.args[i] unless impl_param.external_name == base_param.external_name - @program.report_warning(impl_param, "positional parameter '#{impl_param.external_name}' corresponds to parameter '#{base_param.external_name}' of the overridden method #{Call.def_full_name(base_type, base_method)}, which has a different name and may affect named argument passing") + @program.warnings.try &.add_warning(impl_param, "positional parameter '#{impl_param.external_name}' corresponds to parameter '#{base_param.external_name}' of the overridden method #{Call.def_full_name(base_type, base_method)}, which has a different name and may affect named argument passing") end end end diff --git a/src/compiler/crystal/semantic/ast.cr b/src/compiler/crystal/semantic/ast.cr index 3eee08ca3537..dcdc4dcf91a1 100644 --- a/src/compiler/crystal/semantic/ast.cr +++ b/src/compiler/crystal/semantic/ast.cr @@ -12,15 +12,6 @@ module Crystal ::raise exception_type.for_node(self, message, inner) end - def warning(message, inner = nil, exception_type = Crystal::TypeException) - # TODO extract message formatting from exceptions - String.build do |io| - exception = exception_type.for_node(self, message, inner) - exception.warning = true - exception.append_to_s(io, nil) - end - end - def simple_literal? case self when Nop, NilLiteral, BoolLiteral, NumberLiteral, CharLiteral, diff --git a/src/compiler/crystal/semantic/top_level_visitor.cr b/src/compiler/crystal/semantic/top_level_visitor.cr index dd062f77a515..73542617e4f1 100644 --- a/src/compiler/crystal/semantic/top_level_visitor.cr +++ b/src/compiler/crystal/semantic/top_level_visitor.cr @@ -507,11 +507,11 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor link_annotation = LinkAnnotation.from(ann) if link_annotation.static? - @program.report_warning(ann, "specifying static linking for individual libraries is deprecated") + @program.warnings.try &.add_warning(ann, "specifying static linking for individual libraries is deprecated") end if ann.args.size > 1 - @program.report_warning(ann, "using non-named arguments for Link annotations is deprecated") + @program.warnings.try &.add_warning(ann, "using non-named arguments for Link annotations is deprecated") end type.add_link_annotation(link_annotation) diff --git a/src/compiler/crystal/semantic/warnings.cr b/src/compiler/crystal/semantic/warnings.cr index ebdf7d707e14..059966a4bba0 100644 --- a/src/compiler/crystal/semantic/warnings.cr +++ b/src/compiler/crystal/semantic/warnings.cr @@ -1,68 +1,28 @@ +require "../warnings" + module Crystal class Program - # Which kind of warnings wants to be detected. - property warnings : Warnings = Warnings::All - - # Paths to ignore for warnings detection. - property warnings_exclude : Array(String) = [] of String - - # Detected warning failures. - property warning_failures = [] of String - - # If `true` compiler will error if warnings are found. - property error_on_warnings : Bool = false + # Warning settings and all detected warnings. + property warnings = WarningCollection.new @deprecated_constants_detected = Set(String).new @deprecated_methods_detected = Set(String).new @deprecated_macros_detected = Set(String).new - def report_warning(node : ASTNode, message : String) - return unless self.warnings.all? - return if self.ignore_warning_due_to_location?(node.location) - - self.warning_failures << node.warning(message) - end - - def report_warning_at(location : Location?, message : String) - return unless self.warnings.all? - return if self.ignore_warning_due_to_location?(location) - - if location - message = String.build do |io| - exception = SyntaxException.new message, location.line_number, location.column_number, location.filename - exception.warning = true - exception.append_to_s(io, nil) - end - end - - self.warning_failures << message - end - - def ignore_warning_due_to_location?(location : Location?) - return false unless location - - filename = location.original_filename - return false unless filename - - @program.warnings_exclude.any? do |path| - filename.starts_with?(path) - end - end - def check_deprecated_constant(const : Const, node : Path) - return unless @warnings.all? + return unless @warnings.level.all? check_deprecation(const, node, @deprecated_constants_detected) end def check_call_to_deprecated_macro(a_macro : Macro, call : Call) - return unless @warnings.all? + return unless @warnings.level.all? check_deprecation(a_macro, call, @deprecated_macros_detected) end def check_call_to_deprecated_method(node : Call) - return unless @warnings.all? + return unless @warnings.level.all? return if compiler_expanded_call(node) node.target_defs.try &.each do |target_def| @@ -74,7 +34,7 @@ module Crystal if (ann = object.annotation(self.deprecated_annotation)) && (deprecated_annotation = DeprecatedAnnotation.from(ann)) use_location = use_site.location.try(&.macro_location) || use_site.location - return if !use_location || ignore_warning_due_to_location?(use_location) + return if !use_location || @warnings.ignore_warning_due_to_location?(use_location) # skip warning if the use site was already informed name = object.short_reference @@ -89,7 +49,7 @@ module Crystal end end - @warning_failures << use_site.warning(full_message) + @warnings.infos << use_site.warning(full_message) end end @@ -163,29 +123,4 @@ module Crystal to_s end end - - class Command - def report_warnings - compiler = @compiler - return unless compiler - - program = compiler.program? - return unless program - return if program.warning_failures.empty? - - program.warning_failures.each do |message| - STDERR.puts message - STDERR.puts "\n" - end - STDERR.puts "A total of #{program.warning_failures.size} warnings were found." - end - - def warnings_fail_on_exit? - compiler = @compiler - return false unless compiler - - program = compiler.program - program.error_on_warnings && program.warning_failures.size > 0 - end - end end diff --git a/src/compiler/crystal/warnings.cr b/src/compiler/crystal/warnings.cr new file mode 100644 index 000000000000..8efad8ba4195 --- /dev/null +++ b/src/compiler/crystal/warnings.cr @@ -0,0 +1,110 @@ +module Crystal + # Which warnings to detect. + enum WarningLevel + None + All + end + + # This collection handles warning detection, reporting, and related options. + # It is shared between a `Crystal::Compiler` and other components that need to + # produce warnings. + class WarningCollection + # Which kind of warnings we want to detect. + property level : WarningLevel = :all + + @excluded_paths = [] of String + @lib_path : String? + + # Whether to ignore the "lib" path for warning detection. Turned off by + # the `--exclude-warnings` command-line option. + def exclude_lib_path? : Bool + !@lib_path.nil? + end + + def exclude_lib_path=(exclude : Bool) + @lib_path = exclude ? File.expand_path(Crystal.normalize_path("lib")) : nil + end + + # Detected warnings. + property infos = [] of String + + # Whether the compiler will error if any warnings are detected. + property? error_on_warnings = false + + def exclude_path(path : ::Path | String) + @excluded_paths << File.expand_path(Crystal.normalize_path(path)) + end + + def add_warning(node : ASTNode, message : String) + return unless @level.all? + return if ignore_warning_due_to_location?(node.location) + + @infos << node.warning(message) + end + + def add_warning_at(location : Location?, message : String) + return unless @level.all? + return if ignore_warning_due_to_location?(location) + + if location + message = String.build do |io| + exception = SyntaxException.new message, location.line_number, location.column_number, location.filename + exception.warning = true + exception.append_to_s(io, nil) + end + end + + @infos << message + end + + def report(io : IO) + unless @infos.empty? + @infos.each do |message| + io.puts message + io.puts "\n" + end + io.puts "A total of #{@infos.size} warnings were found." + end + end + + def ignore_warning_due_to_location?(location : Location?) + return false unless location + + filename = location.original_filename + return false unless filename + + if lib_path = @lib_path + return true if filename.starts_with?(lib_path) + end + + @excluded_paths.any? do |path| + filename.starts_with?(path) + end + end + end + + class ASTNode + def warning(message, inner = nil, exception_type = Crystal::TypeException) + # TODO extract message formatting from exceptions + String.build do |io| + exception = exception_type.for_node(self, message, inner) + exception.warning = true + exception.append_to_s(io, nil) + end + end + end + + class Command + def report_warnings + @compiler.try &.warnings.report(STDERR) + end + + def warnings_fail_on_exit? + compiler = @compiler + return false unless compiler + + warnings = compiler.warnings + warnings.error_on_warnings? && !warnings.infos.empty? + end + end +end From b5ca30f1802a90e95b09f1c6d541d842c26b9763 Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Tue, 19 Jul 2022 19:20:15 +0800 Subject: [PATCH 2/2] fixup --- src/compiler/crystal/semantic/abstract_def_checker.cr | 2 +- src/compiler/crystal/semantic/top_level_visitor.cr | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/compiler/crystal/semantic/abstract_def_checker.cr b/src/compiler/crystal/semantic/abstract_def_checker.cr index 8f11c5434da5..7caa9f71678a 100644 --- a/src/compiler/crystal/semantic/abstract_def_checker.cr +++ b/src/compiler/crystal/semantic/abstract_def_checker.cr @@ -375,7 +375,7 @@ class Crystal::AbstractDefChecker impl_param = impl_method.args[i] base_param = base_method.args[i] unless impl_param.external_name == base_param.external_name - @program.warnings.try &.add_warning(impl_param, "positional parameter '#{impl_param.external_name}' corresponds to parameter '#{base_param.external_name}' of the overridden method #{Call.def_full_name(base_type, base_method)}, which has a different name and may affect named argument passing") + @program.warnings.add_warning(impl_param, "positional parameter '#{impl_param.external_name}' corresponds to parameter '#{base_param.external_name}' of the overridden method #{Call.def_full_name(base_type, base_method)}, which has a different name and may affect named argument passing") end end end diff --git a/src/compiler/crystal/semantic/top_level_visitor.cr b/src/compiler/crystal/semantic/top_level_visitor.cr index 73542617e4f1..5aa6b26a19b6 100644 --- a/src/compiler/crystal/semantic/top_level_visitor.cr +++ b/src/compiler/crystal/semantic/top_level_visitor.cr @@ -507,11 +507,11 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor link_annotation = LinkAnnotation.from(ann) if link_annotation.static? - @program.warnings.try &.add_warning(ann, "specifying static linking for individual libraries is deprecated") + @program.warnings.add_warning(ann, "specifying static linking for individual libraries is deprecated") end if ann.args.size > 1 - @program.warnings.try &.add_warning(ann, "using non-named arguments for Link annotations is deprecated") + @program.warnings.add_warning(ann, "using non-named arguments for Link annotations is deprecated") end type.add_link_annotation(link_annotation)