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
participants (3)
-
Daniel Slomovits -
Marcus Denker -
Nicolas Anquetil