• =?UTF-8?Q?Static_analysis_=e2=80=93_Unused_method_return_value_and_?= =

    From Stanimir Stamenkov@21:1/5 to All on Sun May 14 15:38:05 2017
    Is there a static analysis tool (a.k.a. linter) which would flag
    warnings for the following usages:

    some_hash[:a_key].strip.gsub!(/^0+/, '')
    ...

    and then:

    some_collection.map do |foo|
    foo[:bar] == 'baz' && foo[:qux].strip.gsub!(/^0+/, '')
    end

    In the first instance the result of the gsub!() method is
    effectively thrown away resulting in no-op.

    The second case is ambiguous as many of the core Ruby method!()s
    return false, instead of "value" when they haven't actually
    performed a modification. So I would like to have a warning when
    such core ! methods get used in a bigger expressions incl. assignments.

    I've tried RuboCop and RubyCritic but they don't seem to flag the
    given usages as problems. Is there another tool to help with this,
    or is there specific configuration to any of the two mentioned which
    would trigger the intended warnings?

    https://github.com/bbatsov/rubocop
    https://github.com/whitesmith/rubycritic

    --
    Stanimir

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Robert Klemme@21:1/5 to Stanimir Stamenkov on Mon May 15 08:19:03 2017
    On 14.05.2017 14:38, Stanimir Stamenkov wrote:

    I've tried RuboCop and RubyCritic but they don't seem to flag the given usages as problems. Is there another tool to help with this, or is
    there specific configuration to any of the two mentioned which would
    trigger the intended warnings?

    This is hard to do correct as it requires knowledge about how a method
    works. At best, this would only be a rough guideline. It starts by
    reliably detecting the class of the receiver.

    Kind regards

    robert


    --
    remember.guy do |as, often| as.you_can - without end http://blog.rubybestpractices.com/

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Stanimir Stamenkov@21:1/5 to All on Sat Jul 15 18:14:38 2017
    Mon, 15 May 2017 08:19:03 +0200, /Robert Klemme/:
    On 14.05.2017 14:38, Stanimir Stamenkov wrote:

    I've tried RuboCop and RubyCritic but they don't seem to flag the given
    usages as problems. Is there another tool to help with this, or is
    there specific configuration to any of the two mentioned which would
    trigger the intended warnings?

    This is hard to do correct as it requires knowledge about how a
    method works. At best, this would only be a rough guideline. It
    starts by reliably detecting the class of the receiver.

    I'm kinda disappointed in the capabilities of these tools. Most
    reports they produce are about suggested cosmetic formatting, and
    few others which are all guidelines after all. For example, RuboCop
    suggests class variables (vs. class instance variables) should be
    avoided, which doesn't mean they can't have legitimate usage. I've
    seen an experienced Ruby zealot blindly replacing such occurrence
    causing quite substantial bug in the behavior.

    ---

    The examples I've given could be interpreted like:

    some_hash[:a_key].strip.gsub!(/^0+/, '')

    If not defined explicitly elsewhere `strip` is a String instance
    method, and it returns a _new_ string. I think a linter could
    produce quite certain warning the `strip` result is not assigned,
    and chained operations on it are mostly no-op (if that's not the
    last statement of a method). Also the `gsub!` on a temporary string
    is not likely having the desired effect.

    Then, if similar statement/expression is used as a return value of a method/block:

    some_collection.map do |foo|
    foo[:bar].strip.gsub!(/^0+/, '')
    end

    `gsub!` is a String instance method which has the peculiarity of
    returning false, instead of the original string when no substitution
    takes place. This should also be flagged as a warning, as it is
    mostly likely a mistake. It might not be a mistake if there's an
    alternative provided:

    foo[:bar].strip.gsub!(/^0+/, '') || foo[:bar].strip

    Of course this s
  • From Robert Klemme@21:1/5 to Stanimir Stamenkov on Sat Jul 15 18:29:06 2017
    On 15.07.2017 17:14, Stanimir Stamenkov wrote:
    Mon, 15 May 2017 08:19:03 +0200, /Robert Klemme/:
    On 14.05.2017 14:38, Stanimir Stamenkov wrote:

    I've tried RuboCop and RubyCritic but they don't seem to flag the given
    usages as problems. Is there another tool to help with this, or is
    there specific configuration to any of the two mentioned which would
    trigger the intended warnings?

    This is hard to do correct as it requires knowledge about how a method
    works. At best, this would only be a rough guideline. It starts by
    reliably detecting the class of the receiver.

    I'm kinda disappointed in the capabilities of these tools.

    You need to be aware of what is possible at all. Your expectations seem unrealistic high to me.

    Most reports
    they produce are about suggested cosmetic formatting, and few others
    which are all guidelines after all. For example, RuboCop suggests class variables (vs. class instance variables) should be avoided, which
    doesn't mean they can't have legitimate usage.

    But they should really be avoided. I have yet to see a problem that is
    better solved with class variables than with instance variables of the
    class (or even different means).

    I've seen an experienced
    Ruby zealot blindly replacing such occurrence causing quite substantial
    bug in the behavior.

    Any code change can cause bugs (and especially when done with little involvement of the conscious mind). That is the simple truth.

    The examples I've given could be interpreted like:

    some_hash[:a_key].strip.gsub!(/^0+/, '')

    If not defined explicitly elsewhere `strip` is a String instance method,

    The difficulties start by "not explicitly defined elsewhere". The
    method could be in a piece of code that is generated at runtime, could
    be in C etc.

    and it returns a _new_ string. I think a linter could produce quite
    certain warning the `strip` result is not assigned, and chained
    operations on it are mostly no-op (if that's not the last statement of a method). Also the `gsub!` on a temporary string is not likely having
    the desired effect.

    You can say that because you probably have enough knowledge of the
    context. But the fact that something is called "some_hash" does not
    guarantee at all that the variable points to a Hash instance. And it
    goes on. Ruby is so dynamic that you can only know at runtime what
    types are in play etc. Of course, you could produce warnings like you
    suggest but then you'd have to wade through a huge pile of false
    positives. And at the same time some real issues probably go undetected.

    Then, if similar statement/expression is used as a return value of a method/block:

    some_collection.map do |foo|
    foo[:bar].strip.gsub!(/^0+/, '')
    end

    `gsub!` is a String instance method which has the peculiarity of
    returning false,

    No, nil.

    instead of the original string when no substitution
    takes place. This should also be flagged as a warning, as it is mostly likely a mistake. It might not be a mistake if there's an alternative provided:

    ... or if the programmer knows that there will always be at least one substitution.

    foo[:bar].strip.gsub!(/^0+/, '') || foo[:bar].strip

    Of course this suggests the linter maintains some class/method table,
    and has some knowledge of the core Ruby library. I'll see if I could
    pursue for such an enhancement with the RuboCop maintainers.

    I wish you luck!

    Kind regards

    robert

    --
    remember.guy do |as, often| as.you_can - without end http://blog.rubybestpractices.com/

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Stanimir Stamenkov@21:1/5 to All on Sat Jul 15 23:20:11 2017
    Sat, 15 Jul 2017 18:29:06 +0200, /Robert Klemme/:
    On 15.07.2017 17:14, Stanimir Stamenkov wrote:

    I'm kinda disappointed in the capabilities of these tools.

    You need to be aware of what is possible at all. Your expectations
    seem unrealistic high to me.

    If the given use case can't be covered by these tools my conclusion
    of them not being really helpful past formatting style checks would
    remain. I just want to find out if that's the case.

    Most reports they produce are about suggested cosmetic
    formatting, and few others which are all guidelines after all.
    For example, RuboCop suggests class variables (vs. class instance
    variables) should be avoided, which doesn't mean they can't have
    legitimate usage.

    But they should really be avoided. I have yet to see a problem that
    is better solved with class variables than with instance variables
    of the class (or even different means).

    The specific use case was a base class which acts as a registry for
    information provided by its sub types. The base class provides few
    class methods for the registration into a class variable storage
    (that one is protected), and then for reading the info publicly.

    I guess I could make it use class instance variable at the expense I
    should not forget to always invoke the "register" method with
    explicit receiver (the base class) which I don't really want to
    force, and it would be error prone for that reason.

    I guess I could also make the `Base.register` method use `Base.instance_variable_set` but do you think that's anywhere better?

    As far as I know "class instance variables" are quite specific to
    Ruby, so avoiding "class variables" as seen in other languages at
    all cost in favor to what I would call "side-effect class instance
    variables" is confusing to me.

    I've seen an experienced Ruby zealot blindly replacing such
    occurrence causing quite substantial bug in the behavior.

    Any code change can cause bugs (and especially when done with little involvement of the conscious mind). That is the simple truth.

    True. This just a case where it shows the reports produced by these
    tools could be deemed imprecise already, and should not be taken
    lightly (better shut them up).

    The examples I've given could be interpreted like:

    some_hash[:a_key].strip.gsub!(/^0+/, '')

    If not defined explicitly elsewhere `strip` is a String instance
    method,

    The difficulties start by "not explicitly defined elsewhere". The
    method could be in a piece of code that is generated at runtime,
    could be in C etc.

    I'm aware dynamic definitions could lead to false positives, and I
    think I'm suggesting a case where it is less likely to be an issue,
    and more likely to gain advantage of flagging it, unless explicitly
    shut up.

    and it returns a _new_ string. I think a linter could produce
    quite certain warning the `strip` result is not assigned, and
    chained operations on it are mostly no-op (if that's not the last
    statement of a method). Also the `gsub!` on a temporary string is
    not likely having the desired effect.

    You can say that because you probably have enough knowledge of the
    context. But the fact that something is called "some_hash" does not guarantee at all that the variable points to a Hash instance. And
    it goes on. Ruby is so dynamic that you can only know at runtime
    what types are in play etc.

    That's why I've suggested looking up class/method table of explicit definitions, and then the case I'm targeting is specifically about
    the core String#strip (or String#gsub!) method. If there's no other
    type in the project defining `strip` it is really likely the core
    String#strip is used. The chain invocation of `strip.gsub!` could
    further provide a hint to the linter of the actual types used. The
    possibility to imply basic type info, even roughly, seems essential
    to me for such tool to produce helpful reports.

    Of course, you could produce warnings
    like you suggest but then you'd have to wade through a huge pile of
    false positives. And at the same time some real issues probably go undetected.

    As far as the false positives are much fewer than the real problems
    caught by such a report, shutting them up via the comment annotation
    mechanism provided by the tool seems fine to me. The statement "I
    would have to wade through a huge pile of false positives" is yet to
    be determined.

    Then, if similar statement/expression is used as a return value of
    a method/block:

    some_collection.map do |foo|
    foo[:bar].strip.gsub!(/^0+/, '')
    end

    `gsub!` is a String instance method which has the peculiarity of
    returning false,

    No, nil.

    Thanks for correcting.

    instead of the original string when no substitution takes place.
    This should also be flagged as a warning, as it is mostly likely a
    mistake. It might not be a mistake if there's an alternative
    provided:

    ... or if the programmer knows that there will always be at least
    one substitution.

    Whether a programmer is certain is questionable – that's the linter
    purpose. It's better the programmer annotate this case explicitly
    flagging his/her assurance of it. Then after the original
    programmer is gone the annotation would remain as proof.

    foo[:bar].strip.gsub!(/^0+/, '') || foo[:bar].strip

    Of course this suggests the linter maintains some class/method
    table, and has some knowledge of the core Ruby library. I'll see
    if I could pursue for such an enhancement with the RuboCop
    maintainers.

    I wish you luck!

    Thank you. It's not likely I'll do it too soon, not unless I could
    prepare a formal proposal with more examples, and possible behavior.

    --
    Stanimir

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)