`Lint/UselessAssign` reports macros accepting `Crystal::Macros::TypeDeclaration` about ameba

[Blacksmoke16]

It just feels less than ideal that new rules keep being added that cannot be actually robust without semantic analysis, or are more subjective in general. Which ends up forcing me to either disable them inline, globally, or conform to the suggestions on every minor release.

I still think some/most of these should be disabled by default, or made optional via some sort of opinionated style extension.

[Blacksmoke16]

@Sija I think this needs to be reopened, doesn't seem resolved from what I can tell:

$ git rev-parse HEAD
590640b559875aa9bb76783be95126dd07d9ed27

$ make
shards build -Dpreview_mt
Dependencies are satisfied
Building: ameba

$ cat test.cr 
macro test(type_decl)
  def {{type_decl.var.id}} : {{type_decl.type}}
    "foo"
  end
end

test name : String

$ ./bin/ameba test.cr 
Inspecting 1 file

F

test.cr:7:6
[W] Lint/UselessAssign: Useless assignment to variable `name`
> test name : String
       ^--^

Finished in 21.03 milliseconds
1 inspected, 1 failure

[Sija]

This is one of the cases that IMO cannot be fixed at the level of analysis that ameba is doing. You can either use inline pragma, exclude the file on the project level or turn on the ExcludeTypeDeclarations rule option.

I was thinking of AllowedMacroNames to be able to whitelist them at the project level, although I'm not convinced whether it's worth implementing, when in most of the cases you'd probably use ExcludeTypeDeclarations.

[straight-shoota]

I think there is indeed some room for improvement, even without semantic analysis.

For example, the rule could take into account if the assignment / type declaration is located in a call argument. That would be a strong indicator for a macro call. Using a local var assignment or even a type declaration in an argument would be quite unusual and certainly not well-formed code.
So this would be an exclusion criterion. Of course there's a chance for underreporting because the macro might still make the type declaration for a local variable. But that's unlikely. And I think it's an acceptable compromise and better than having to explicitly disable this rule in general or for many locations.

Also I'm still wondering why class Foo; getter name : String; end is not reported (ref #429 (comment)).

[straight-shoota]

The other week I looked into recognition of flag? macro calls (c.f. https://github.com/crystal-lang/crystal/pull/14234).[^1] This requires a similar contextual awareness for being in a macro context. That's pretty similar to a call arg context.

I solved this by declaring a property in_macro_expression on the rule which is then assigned by the visitor (if it exists).

# in rule/xyz.cr
    # Returns `true` if the visitor is currently inside a macro expression.
    @[YAML::Field(ignore: true)]
    property? in_macro_expression : Bool = false

# in src/ameba/ast/visitors/node_visitor.cr
    def visit(node : Crystal::MacroExpression | Crystal::MacroIf | Crystal::MacroFor)
      rule = @rule
      if rule.responds_to?(:in_macro_expression=)
        rule.in_macro_expression = true
      end

      !skip?(node)
    end

    def end_visit(node : Crystal::MacroExpression | Crystal::MacroIf | Crystal::MacroFor)
      rule = @rule
      if rule.responds_to?(:in_macro_expression=)
        rule.in_macro_expression = false
      end
    end

I figure this could work pretty similarly for Call.
Considering the similar use case for call contexts, it might even make sense to generalize this as a context stack.

[hugopl]

Answering to my own question, yes, this options was introduced on 1.6.1.

CI here runs an older version and I got:

Error: Unable to load config file: Unknown yaml attribute: ExcludeTypeDeclarations at line 2, column 1

But yes, it solved the warnings with Avran, OTOH it loses the ability to detect unused variables declared like:

a : Int32? = nil

[Sija]

@straight-shoota Excluding call arguments might work, let's try that 👍

[Sija]

@straight-shoota Here you go!

Also I'm still wondering why class Foo; getter name : String; end is not reported (ref #429 (comment)).

ameba/src/ameba/ast/visitors/scope_visitor.cr

Lines 178 to 179 in 28fafea

	when scope.type_definition? && accessor_macro?(node)
	return false

Just FTR, note that the example is different from the one posted in #429 (comment).

@Blacksmoke16 These are definitely topics up for a discussion. Personally, I've had problem with the DocumentationAdmonition rule, which popped flags across many of my projects and tbh I was gonna disable it by default, the UselessAssign otoh was pretty solid until recent expansion into the type declaration territory which allowed for a few bugs to creep in. And yes, lack of semantic analysis is real PITA, I'd love to get this resolved, since it's a biggest issue towards robustness and reliability. With that we can easily minimize the amount of false positives.

[straight-shoota]

@Sija Awesome!

With the changes from #450, would accessor_macro? be obsolete? I'm assuming its sole purpose was to filter usless assigns in macro arguments which is no handled generically. It might have other reasons, though.

[Sija]

@straight-shoota nope, they're still needed (to handle record Bar, foo = 42 type of cases).

[Sija]

@Blacksmoke16 you're right, seems that my naive heuristics broke:

ameba/src/ameba/rule/lint/useless_assign.cr

Lines 57 to 60 in 590640b

	private def expressions_with_call?(node)
	node.is_a?(Crystal::Expressions) &&
	node.expressions.first?.is_a?(Crystal::Call)
	end

[Sija]

Proper fix would most likely have to touch the ScopeVisitor logic, since atm its inner workings make it hard or impossible to resolve this issue - i.e. I see no way to sensibly correlate whether a specific type definition is being used as a part of a call.

[hugopl]

Avram models uses macros like these to declare the database columns, so the very useful Lint/UselessAssign rule need to be disabled on all models if using Avran 😢.

[Sija]

@hugopl That's a bummer indeed. OTOH IIUC the ExcludeTypeDeclarations: true option should cover majority of the cases, doesn't it?

[hugopl]

I didn't know about this option, I'll give it a try, thanks.

Was this option introduced with the new behavior? Or was it always there?

[jwoertink]

Just adding to this since I just upgraded Ameba on Lucky. The entire Lucky ecosystem gets hit by it since all Habitat configs, params, and use of needs everywhere are all identified. For now I'll just set ExcludeTypeDeclarations: true option so we can still get all the other goodies 😄