ensure: and assignments RB Rule
Locked
 
|
Hi guys
I'm revisiting all the RB rule and I'm puzzled by this one. Checks assignment to a variable that is the first statement inside the value block that is also used in the unwind block. [| `@temps | `var := `@object. `@.statements] ensure: [`var `@messages: `@args] Does anybody have an idea? Stef |
|
|
As the class of this rule is named RBFileBlocksRule, I guess it comes from some practice used when working with files. I’m not a file guru, but maybe others can tell more.
Stef, as you are working on rules, can we merge category and group of the rule? I understand that one was added later by Manifest, but it is in the Pharo core anyway… Uko > On 29 Nov 2014, at 21:41, stepharo <[hidden email]> wrote: > > Hi guys > > I'm revisiting all the RB rule and I'm puzzled by this one. > > > Checks assignment to a variable that is the first statement inside the value block that is also used in the unwind block. > > > [| `@temps | > `var := `@object. > `@.statements] > ensure: > [`var `@messages: `@args] > > Does anybody have an idea? > > Stef > |
|
|
The code would look like this:
[ | file | file := StandardFileStream forceNewFileNamed: ‘foo’. file nextPutAll: ‘bar’ ] ensure: [ file close ]. Why that is considered bad practice however, I can’t tell. > On 29.11.2014, at 23:01, Yuriy Tymchuk <[hidden email]> wrote: > > As the class of this rule is named RBFileBlocksRule, I guess it comes from some practice used when working with files. I’m not a file guru, but maybe others can tell more. > > Stef, as you are working on rules, can we merge category and group of the rule? I understand that one was added later by Manifest, but it is in the Pharo core anyway… > > Uko > >> On 29 Nov 2014, at 21:41, stepharo <[hidden email]> wrote: >> >> Hi guys >> >> I'm revisiting all the RB rule and I'm puzzled by this one. >> >> >> Checks assignment to a variable that is the first statement inside the value block that is also used in the unwind block. >> >> >> [| `@temps | >> `var := `@object. >> `@.statements] >> ensure: >> [`var `@messages: `@args] >> >> Does anybody have an idea? >> >> Stef >> > > |
Re: ensure: and assignments RB Rule
|
|
Le 30/11/2014 11:06, Max Leske a écrit :
> The code would look like this: > > [ | file | > file := StandardFileStream forceNewFileNamed: ‘foo’. > file nextPutAll: ‘bar’ ] > ensure: [ file close ]. > > Why that is considered bad practice however, I can’t tell. This one would fail because file is a temp to the block (and not visible to the ensure block). Another problem is that, if you have a failure in forceNewFileNamed:, file is nil and the ensure: fail as well. This is how I interpret that rule. Thierry |
Re: ensure: and assignments RB Rule
|
|
This is how it should be done:
readStreamDo: aBlock | stream | stream := self readStream. ^ [ aBlock value: stream ] ensure: [ stream close ] Errors while opening or closing are still signalled, which is good IMO. > On 30 Nov 2014, at 11:18, Thierry Goubier <[hidden email]> wrote: > > Le 30/11/2014 11:06, Max Leske a écrit : >> The code would look like this: >> >> [ | file | >> file := StandardFileStream forceNewFileNamed: ‘foo’. >> file nextPutAll: ‘bar’ ] >> ensure: [ file close ]. >> >> Why that is considered bad practice however, I can’t tell. > > This one would fail because file is a temp to the block (and not visible to the ensure block). > > Another problem is that, if you have a failure in forceNewFileNamed:, file is nil and the ensure: fail as well. > > This is how I interpret that rule. > > Thierry > > |
|
|
In reply to this post by Thierry Goubier
> On 30.11.2014, at 11:18, Thierry Goubier <[hidden email]> wrote: > > Le 30/11/2014 11:06, Max Leske a écrit : >> The code would look like this: >> >> [ | file | >> file := StandardFileStream forceNewFileNamed: ‘foo’. >> file nextPutAll: ‘bar’ ] >> ensure: [ file close ]. >> >> Why that is considered bad practice however, I can’t tell. > > This one would fail because file is a temp to the block (and not visible to the ensure block). If it weren’t visible, wouldn’t the compiler raise an exception? > > Another problem is that, if you have a failure in forceNewFileNamed:, file is nil and the ensure: fail as well. True, but that’s a problem this rule doesn’t really cover I guess. That one’s up to the developer. > > This is how I interpret that rule. > > Thierry > > |
Re: ensure: and assignments RB Rule
|
|
Le 30/11/2014 12:01, Max Leske a écrit :
> >> On 30.11.2014, at 11:18, Thierry Goubier <[hidden email]> wrote: >> >> Le 30/11/2014 11:06, Max Leske a écrit : >>> The code would look like this: >>> >>> [ | file | >>> file := StandardFileStream forceNewFileNamed: ‘foo’. >>> file nextPutAll: ‘bar’ ] >>> ensure: [ file close ]. >>> >>> Why that is considered bad practice however, I can’t tell. >> >> This one would fail because file is a temp to the block (and not visible to the ensure block). > > If it weren’t visible, wouldn’t the compiler raise an exception? In your example, yes. But the rule matched: | file | [ file := StandardFileStream forceNewFileNamed: ‘foo’. file nextPutAll: ‘bar’ ] ensure: [ file close ]. (Hum: wouldn't the compiler complain of file might be nil in the ensure block?) >> Another problem is that, if you have a failure in forceNewFileNamed:, file is nil and the ensure: fail as well. > > True, but that’s a problem this rule doesn’t really cover I guess. That one’s up to the developer. I would expect that rule to flag bad coding practices, and I think it does ;) Thierry |
|
|
thanks we should open a bug entry to improve the class comment of this rule.
Any taker? Stef Le 30/11/14 12:10, Thierry Goubier a écrit : > Le 30/11/2014 12:01, Max Leske a écrit : >> >>> On 30.11.2014, at 11:18, Thierry Goubier <[hidden email]> >>> wrote: >>> >>> Le 30/11/2014 11:06, Max Leske a écrit : >>>> The code would look like this: >>>> >>>> [ | file | >>>> file := StandardFileStream forceNewFileNamed: ‘foo’. >>>> file nextPutAll: ‘bar’ ] >>>> ensure: [ file close ]. >>>> >>>> Why that is considered bad practice however, I can’t tell. >>> >>> This one would fail because file is a temp to the block (and not >>> visible to the ensure block). >> >> If it weren’t visible, wouldn’t the compiler raise an exception? > > In your example, yes. > > But the rule matched: > > | file | > [ > file := StandardFileStream forceNewFileNamed: ‘foo’. > file nextPutAll: ‘bar’ ] > ensure: [ file close ]. > > (Hum: wouldn't the compiler complain of file might be nil in the > ensure block?) > >>> Another problem is that, if you have a failure in >>> forceNewFileNamed:, file is nil and the ensure: fail as well. >> >> True, but that’s a problem this rule doesn’t really cover I guess. >> That one’s up to the developer. > > I would expect that rule to flag bad coding practices, and I think it > does ;) > > Thierry > > |
Re: ensure: and assignments RB Rule
|
|
2014-11-30 20:54 GMT+01:00 stepharo <[hidden email]>: thanks we should open a bug entry to improve the class comment of this rule. done Add a comment describing RBFileBlocksRule now I just need to have a good description for this rule. Please, can someone outline the discussion on this thread: - describe why code matching this rule is considered back practice - describe how the code could be rewritten. nicolai
|
Re: ensure: and assignments RB Rule
|
|
Anyone? We may remove this rule if no one understands for what this is good for. (if anyone knows, please comment that class).2014-12-14 11:22 GMT+01:00 Nicolai Hess <[hidden email]>:
|
Re: ensure: and assignments RB Rule
|
|
If there's an error in the expression that is assigned to file, and the stack unwound, file won't have been assigned and the close will get sent to nil. The assignment to file should precede the block. Sent from my iPhone
|
Re: ensure: and assignments RB Rule
|
|
Thank you all for your response, now the interesting question is, what should this rule recommend to use instead? First assign to a block local variable andadd a nil check in the unwind block? And maybe we should improve that rule, because in Pharo 5.0 there are 5 methods catched by 2015-08-31 15:30 GMT+02:00 Eliot Miranda <[hidden email]>:
|
Re: ensure: and assignments RB Rule
|
|
On Tue, Sep 1, 2015 at 1:29 PM, Nicolai Hess <[hidden email]> wrote:
No. I think the pattern modifies this: | t | ... [t := initExpr. exprs] ensure: [t msg] to this | t | ... t := initExpr. [exprs] ensure: [t msg] Control never reaches the block [exprs] isf there is an error in initExpr so there's no issue with t being unassigned; control won't get to the ensure block, so no need to test for nil.
_,,,^..^,,,_ best, Eliot |
Re: ensure: and assignments RB Rule
|
|
*Now* I understand this rule. 2015-09-02 3:40 GMT+02:00 Eliot Miranda <[hidden email]>:
|
Re: ensure: and assignments RB Rule
|
|
Sent from my iPhone
|
«
Return to Pharo Smalltalk Developers
|
1 view|%1 views
| Free forum by Nabble | Edit this page |