QA rule suggestion - assignment symbol slips

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

QA rule suggestion - assignment symbol slips

Ben Coman
A quick suggestion for anyone working on QA rules...

Just now I am playing with FFI a bit and switching back a forth from
Pharo to C and sometimes I end up doing...
       location = Libclang clang_getRangeStart__range: range.

thus QA reported a. "Temporaries read before written"
and b. "Temporary variables not read or not written"
but these weren't helpful since I was stuck staring at it saying to myself
   "Well range is assigned before its used, and location is assigned,
so bah! the rules seem broken"
and for about 30 seconds I remained blind to the truth I'd left the
colon off the assignment operator.

I'd expect newcomers from other languages might slip in the same way,
so a higher priority rule extending [b.] that identifies such variable
followed by equal-sign would provide pertinent feedback.

cheers -ben

Reply | Threaded
Open this post in threaded view
|

Re: QA rule suggestion - assignment symbol slips

Marcus Denker-4
Hi,

The  "Temporaries read before written” rule is implemented very in a way that you get
*a lot* of false positives.

e.g. it treats *any* block as potentially no exectuted, so any code like

messageSelectorAndArgumentNames
        | t |
        self doSomething: [ t:=1 ].
        ^t

or

messageSelectorAndArgumentNames
        | t |
        self init: t.
        ^t

triggers the rule. There are many false positives.

I sometimes wonder if this rule makes sense with this strict interpretation.

It leads to bug tracker entries like this:

https://pharo.fogbugz.com/f/cases/18943/PositionableStream-upTo-raised-temp-read-before-written

        Marcus


> On 28 Nov 2017, at 02:54, Ben Coman <[hidden email]> wrote:
>
> A quick suggestion for anyone working on QA rules...
>
> Just now I am playing with FFI a bit and switching back a forth from
> Pharo to C and sometimes I end up doing...
>       location = Libclang clang_getRangeStart__range: range.
>
> thus QA reported a. "Temporaries read before written"
> and b. "Temporary variables not read or not written"
> but these weren't helpful since I was stuck staring at it saying to myself
>   "Well range is assigned before its used, and location is assigned,
> so bah! the rules seem broken"
> and for about 30 seconds I remained blind to the truth I'd left the
> colon off the assignment operator.
>
> I'd expect newcomers from other languages might slip in the same way,
> so a higher priority rule extending [b.] that identifies such variable
> followed by equal-sign would provide pertinent feedback.
>
> cheers -ben
>


Reply | Threaded
Open this post in threaded view
|

Re: QA rule suggestion - assignment symbol slips

Marcus Denker-4
as for catching message sends of the kind

        var = 1 + 2.

(instead of :=), yes, we can add a rule for that.

> On 28 Nov 2017, at 09:20, Marcus Denker <[hidden email]> wrote:
>
> Hi,
>
> The  "Temporaries read before written” rule is implemented very in a way that you get
> *a lot* of false positives.
>
> e.g. it treats *any* block as potentially no exectuted, so any code like
>
> messageSelectorAndArgumentNames
> | t |
> self doSomething: [ t:=1 ].
> ^t
>
> or
>
> messageSelectorAndArgumentNames
> | t |
> self init: t.
> ^t
>
> triggers the rule. There are many false positives.
>
> I sometimes wonder if this rule makes sense with this strict interpretation.
>
> It leads to bug tracker entries like this:
>
> https://pharo.fogbugz.com/f/cases/18943/PositionableStream-upTo-raised-temp-read-before-written
>
> Marcus
>
>
>> On 28 Nov 2017, at 02:54, Ben Coman <[hidden email]> wrote:
>>
>> A quick suggestion for anyone working on QA rules...
>>
>> Just now I am playing with FFI a bit and switching back a forth from
>> Pharo to C and sometimes I end up doing...
>>      location = Libclang clang_getRangeStart__range: range.
>>
>> thus QA reported a. "Temporaries read before written"
>> and b. "Temporary variables not read or not written"
>> but these weren't helpful since I was stuck staring at it saying to myself
>>  "Well range is assigned before its used, and location is assigned,
>> so bah! the rules seem broken"
>> and for about 30 seconds I remained blind to the truth I'd left the
>> colon off the assignment operator.
>>
>> I'd expect newcomers from other languages might slip in the same way,
>> so a higher priority rule extending [b.] that identifies such variable
>> followed by equal-sign would provide pertinent feedback.
>>
>> cheers -ben
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: QA rule suggestion - assignment symbol slips

Marcus Denker-4
In reply to this post by Marcus Denker-4

>
> or
>
> messageSelectorAndArgumentNames
> | t |
> self init: t.
> ^t
>
This one is good to warn… but we have the “unitialized var”
rule for that already.

        Marcus


Reply | Threaded
Open this post in threaded view
|

Re: QA rule suggestion - assignment symbol slips

Ben Coman
In reply to this post by Marcus Denker-4
On 28 November 2017 at 16:25, Marcus Denker <[hidden email]> wrote:
> as for catching message sends of the kind
>
>         var = 1 + 2.
>
> (instead of :=), yes, we can add a rule for that.

Cool. I added https://pharo.fogbugz.com/f/cases/20780


>
>> On 28 Nov 2017, at 09:20, Marcus Denker <[hidden email]> wrote:
>>
>> Hi,
>>
>> The  "Temporaries read before written” rule is implemented very in a way that you get
>> *a lot* of false positives.
>>
>> e.g. it treats *any* block as potentially no exectuted, so any code like
>>
>> messageSelectorAndArgumentNames
>>       | t |
>>       self doSomething: [ t:=1 ].
>>       ^t
>>
>> or
>>
>> messageSelectorAndArgumentNames
>>       | t |
>>       self init: t.
>>       ^t
>>
>> triggers the rule. There are many false positives.
>>
>> I sometimes wonder if this rule makes sense with this strict interpretation.
>>
>> It leads to bug tracker entries like this:
>>
>> https://pharo.fogbugz.com/f/cases/18943/PositionableStream-upTo-raised-temp-read-before-written
>>
>>       Marcus
>>
>>
>>> On 28 Nov 2017, at 02:54, Ben Coman <[hidden email]> wrote:
>>>
>>> A quick suggestion for anyone working on QA rules...
>>>
>>> Just now I am playing with FFI a bit and switching back a forth from
>>> Pharo to C and sometimes I end up doing...
>>>      location = Libclang clang_getRangeStart__range: range.
>>>
>>> thus QA reported a. "Temporaries read before written"
>>> and b. "Temporary variables not read or not written"
>>> but these weren't helpful since I was stuck staring at it saying to myself
>>>  "Well range is assigned before its used, and location is assigned,
>>> so bah! the rules seem broken"
>>> and for about 30 seconds I remained blind to the truth I'd left the
>>> colon off the assignment operator.
>>>
>>> I'd expect newcomers from other languages might slip in the same way,
>>> so a higher priority rule extending [b.] that identifies such variable
>>> followed by equal-sign would provide pertinent feedback.
>>>
>>> cheers -ben
>>>
>>
>
>