Comments (16)
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.
from ameba.
@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
from ameba.
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
.
from ameba.
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)).
from ameba.
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.
from ameba.
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
from ameba.
@straight-shoota Excluding call arguments might work, let's try that 👍
from ameba.
@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
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.
from ameba.
@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.
from ameba.
@straight-shoota nope, they're still needed (to handle record Bar, foo = 42
type of cases).
from ameba.
@Blacksmoke16 you're right, seems that my naive heuristics broke:
ameba/src/ameba/rule/lint/useless_assign.cr
Lines 57 to 60 in 590640b
from ameba.
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.
from ameba.
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 😢.
from ameba.
@hugopl That's a bummer indeed. OTOH IIUC the ExcludeTypeDeclarations: true
option should cover majority of the cases, doesn't it?
from ameba.
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?
from ameba.
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 😄
from ameba.
Related Issues (20)
- warn on redundant use of `.try` HOT 3
- Recognize unused variable from type declaration HOT 2
- ComparisonToBoolean with Bool unions HOT 2
- exception names are too limited HOT 7
- A dedicated `rules` command HOT 1
- `Lint/UselessAssign` reports type declarations within `lib` definitions
- Add lsp server option HOT 1
- `Lint/UselessAssign` when using generics HOT 9
- Avoid annoyance from new rules HOT 4
- Naming/BlockParameterName + stdlib methods like sort HOT 4
- spec helpers getting flagged HOT 1
- Custom macro generates "Useless assignment to variable" HOT 3
- Naming/BlockParameterName with ignored names HOT 1
- Ameba trips on folder named `*.cr`
- [Feature Request] Unused `rescue` variable name
- [Feature Request] Lint against accessing instance variables outside of the instance of a class HOT 4
- ameba no longer works in WSL
- Add rule enforcing explicit return HOT 5
- False positive `Lint/UselessAssign`
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from ameba.