Code quality: tagging unsent selectors (e.g. in tests)
Hi,
If you have in your code a message send with a selector that is not implemented anywhere, it likely will result in a DNU
- but not if you compile a method on the fly (used in tests often)
- if the DNU is catched (e.g. for a proxy pattern)
- if the code is compatibility code, never executed in this version of the system
We can ban the rule, but that just stopps checking for the whole method for all selectors, what we want is to communicate that this selector is ok in this
special use (per method granularity is ok).
SystemNaviation>>#allUnimplementedCalls did have the feature that one can tag selectors that should be ignored. This uses the pragma #ignoreUnimplementedCalls:
see
Context class >> initializePrimitiveSimulators
<ignoreUnimplementedCalls: #(#registerPrimitiveSimulators)>
This was used in just one method, I think because a) the rule did not take it into account and b) the terminology is just wrong (“Call”).
I think
- ReSentNotImplementedRule should takes this pragma into account
- change the bad terminology “allUnimplementedCalls"
- use pragma: <ignoreNotImplementedSelectors: >
PR is here: https://github.com/pharo-project/pharo/pull/12295
(what the PR does not yet do is to change syntax highlight to not color the selector red, this could be done later, too)
correct me if I am wrong, but one can already deactivate a rule in
Nautilus on a perpackage/class/method basis.
So why not use this mechanism instead of having yet another pragma
(there is a whole universe of pragmas hidden in the code)
nicolas
On 26/01/2023 10:46, Marcus Denker wrote:
Hi,
If you have in your code a message send with a selector that is not implemented anywhere, it likely will result in a DNU
- but not if you compile a method on the fly (used in tests often)
- if the DNU is catched (e.g. for a proxy pattern)
- if the code is compatibility code, never executed in this version of the system
We can ban the rule, but that just stopps checking for the whole method for all selectors, what we want is to communicate that this selector is ok in this
special use (per method granularity is ok).
SystemNaviation>>#allUnimplementedCalls did have the feature that one can tag selectors that should be ignored. This uses the pragma #ignoreUnimplementedCalls:
see
Context class >> initializePrimitiveSimulators
<ignoreUnimplementedCalls: #(#registerPrimitiveSimulators)>
This was used in just one method, I think because a) the rule did not take it into account and b) the terminology is just wrong (“Call”).
I think
- ReSentNotImplementedRule should takes this pragma into account
- change the bad terminology “allUnimplementedCalls"
- use pragma: <ignoreNotImplementedSelectors: >
PR is here: https://github.com/pharo-project/pharo/pull/12295
(what the PR does not yet do is to change syntax highlight to not color the selector red, this could be done later, too)
--
Nicolas Anquetil
RMod team -- Inria Lille
On 26 Jan 2023, at 11:21, Nicolas Anquetil nicolas.anquetil@inria.fr wrote:
correct me if I am wrong, but one can already deactivate a rule in Nautilus on a perpackage/class/method basis.
So why not use this mechanism instead of having yet another pragma
Because disabeling the rule disables it for all selectors
(there is a whole universe of pragmas hidden in the code)
But the pragma is less hidden than the banned rule, which is completely invisible.
Marcus
On 26 Jan 2023, at 11:27, Marcus Denker marcus.denker@inria.fr wrote:
On 26 Jan 2023, at 11:21, Nicolas Anquetil nicolas.anquetil@inria.fr wrote:
correct me if I am wrong, but one can already deactivate a rule in Nautilus on a perpackage/class/method basis.
So why not use this mechanism instead of having yet another pragma
Because disabeling the rule disables it for all selectors
(there is a whole universe of pragmas hidden in the code)
But the pragma is less hidden than the banned rule, which is completely invisible.
The whole rule banning meta data storage is very very limited right now… e.g it does never get cleaned, is never updated
if the code it annotates changes and has many problems.
The long-term solution I want to do is to improve meta-data for code storage and use it for the rule banning
and for things we might abuse pragmas now.
But that will no be soon, and there are lots of things to be taken into account for a nice design.
For me the pragma is a nice intermediate solution: per selector, survives rename of method, rename of
class, move method, is visible…
Marcus
A bit of perspective on other approaches to this problem:
In Dolphin Smalltalk, the FFI mechanism uses #doesNotUnderstand: to
implement getters and setters on external structs, so it already has a
mechanism for ensuring they aren't flagged as unimplemented. In addition to
the standard ClassDescription>>selectors, which is more-or-less the actual
keys of the methodDictionary, there is #understoodSelectors, which is
explicitly a hook to include others understood via #doesNotUnderstand:.
These are then treated exactly as actually-implemented selectors, that is,
they are available for autocomplete as well as not being flagged as
unimplemented.
Also, I believe the unimplemented-selectors rule applies only to
non-self-sends, and there is a separate rule for self-sends which instead
just asks the class if it #canUnderstand: the selector, which creates a
middle ground where the selector is not "known" to the system at large, but
won't be warned about if self-sent.
The pragma approach has the advantage of being more specific, which is
useful if a certain selector is valid only in a very narrow context and
should still be warned about in general. Conversely, the disadvantage is
that it needs to be repeated everywhere the message is sent, rather than
centralized on the class where it is implemented/understood. I assume it
also does not influence autocomplete. So, for cases that are valid in a
broader scope, it creates the extra work of adding the pragma, and you
have to type out the actual send manually, multiplied for each time it's
used—lots of repetition. Fundamentally we're running up against the
limitations of a dynamically-typed language here.
Long story short, I think both options may be useful. In my own usage I
find it's about 50/50—some cases of one-offs in a test, other cases of a
widely-used class with some dynamic behavior. I do like the pragma as an
improvement over rule banning for sure. Looking towards improving rule
banning, I think it's very important for that to be (a) highly visible (and
nothing is more visible than something that's part of the source code) and
(b) manageable via source control (which metadata certainly can
be—categories,
for instance, already are). Call me old-fashioned, but for all the wonders
of the image, there's something to be said for having a limited set of
concepts in a "dead text" format that fully describe everything about the
code—and if there are too many separate pieces of UI presenting
information, it's easy to forget that some of them exist. But this is a
discussion for another thread I think.
On Thu, Jan 26, 2023 at 5:48 AM Marcus Denker marcus.denker@inria.fr
wrote:
On 26 Jan 2023, at 11:27, Marcus Denker marcus.denker@inria.fr wrote:
On 26 Jan 2023, at 11:21, Nicolas Anquetil nicolas.anquetil@inria.fr
wrote:
correct me if I am wrong, but one can already deactivate a rule in
Nautilus on a perpackage/class/method basis.
So why not use this mechanism instead of having yet another pragma
Because disabeling the rule disables it for all selectors
(there is a whole universe of pragmas hidden in the code)
But the pragma is less hidden than the banned rule, which is completely
invisible.
The whole rule banning meta data storage is very very limited right now…
e.g it does never get cleaned, is never updated
if the code it annotates changes and has many problems.
The long-term solution I want to do is to improve meta-data for code
storage and use it for the rule banning
and for things we might abuse pragmas now.
But that will no be soon, and there are lots of things to be taken into
account for a nice design.
For me the pragma is a nice intermediate solution: per selector, survives
rename of method, rename of
class, move method, is visible…
Marcus
Thanks, I will take some notes for a future “better code meta data” project.
For now I propose to merge the PR as a first step.
Marcus
On 26 Jan 2023, at 18:52, Daniel Slomovits daniels220@gmail.com wrote:
A bit of perspective on other approaches to this problem:
In Dolphin Smalltalk, the FFI mechanism uses #doesNotUnderstand: to implement getters and setters on external structs, so it already has a mechanism for ensuring they aren't flagged as unimplemented. In addition to the standard ClassDescription>>selectors, which is more-or-less the actual keys of the methodDictionary, there is #understoodSelectors, which is explicitly a hook to include others understood via #doesNotUnderstand:. These are then treated exactly as actually-implemented selectors, that is, they are available for autocomplete as well as not being flagged as unimplemented.
Also, I believe the unimplemented-selectors rule applies only to non-self-sends, and there is a separate rule for self-sends which instead just asks the class if it #canUnderstand: the selector, which creates a middle ground where the selector is not "known" to the system at large, but won't be warned about if self-sent.
The pragma approach has the advantage of being more specific, which is useful if a certain selector is valid only in a very narrow context and should still be warned about in general. Conversely, the disadvantage is that it needs to be repeated everywhere the message is sent, rather than centralized on the class where it is implemented/understood. I assume it also does not influence autocomplete. So, for cases that are valid in a broader scope, it creates the extra work of adding the pragma, and you have to type out the actual send manually, multiplied for each time it's used—lots of repetition. Fundamentally we're running up against the limitations of a dynamically-typed language here.
Long story short, I think both options may be useful. In my own usage I find it's about 50/50—some cases of one-offs in a test, other cases of a widely-used class with some dynamic behavior. I do like the pragma as an improvement over rule banning for sure. Looking towards improving rule banning, I think it's very important for that to be (a) highly visible (and nothing is more visible than something that's part of the source code) and (b) manageable via source control (which metadata certainly can be—categories, for instance, already are). Call me old-fashioned, but for all the wonders of the image, there's something to be said for having a limited set of concepts in a "dead text" format that fully describe everything about the code—and if there are too many separate pieces of UI presenting information, it's easy to forget that some of them exist. But this is a discussion for another thread I think.
On Thu, Jan 26, 2023 at 5:48 AM Marcus Denker <marcus.denker@inria.fr mailto:marcus.denker@inria.fr> wrote:
On 26 Jan 2023, at 11:27, Marcus Denker <marcus.denker@inria.fr mailto:marcus.denker@inria.fr> wrote:
On 26 Jan 2023, at 11:21, Nicolas Anquetil <nicolas.anquetil@inria.fr mailto:nicolas.anquetil@inria.fr> wrote:
correct me if I am wrong, but one can already deactivate a rule in Nautilus on a perpackage/class/method basis.
So why not use this mechanism instead of having yet another pragma
Because disabeling the rule disables it for all selectors
(there is a whole universe of pragmas hidden in the code)
But the pragma is less hidden than the banned rule, which is completely invisible.
The whole rule banning meta data storage is very very limited right now… e.g it does never get cleaned, is never updated
if the code it annotates changes and has many problems.
The long-term solution I want to do is to improve meta-data for code storage and use it for the rule banning
and for things we might abuse pragmas now.
But that will no be soon, and there are lots of things to be taken into account for a nice design.
For me the pragma is a nice intermediate solution: per selector, survives rename of method, rename of
class, move method, is visible…
Marcus