ensure: and assignments RB Rule
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 <stepharo@free.fr> 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 <yuriy.tymchuk@me.com> 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 <stepharo@free.fr> 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
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
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 <thierry.goubier@gmail.com> 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
On 30.11.2014, at 11:18, Thierry Goubier <thierry.goubier@gmail.com> 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
Le 30/11/2014 12:01, Max Leske a écrit :
On 30.11.2014, at 11:18, Thierry Goubier <thierry.goubier@gmail.com> 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 <thierry.goubier@gmail.com> 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
2014-11-30 20:54 GMT+01:00 stepharo <stepharo@free.fr>:
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 :
done 14618 <https://pharo.fogbugz.com/default.asp?14618> 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
Le 30/11/2014 12:01, Max Leske a écrit :
On 30.11.2014, at 11:18, Thierry Goubier <thierry.goubier@gmail.com>
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
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 <nicolaihess@web.de>:
2014-11-30 20:54 GMT+01:00 stepharo <stepharo@free.fr>:
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 :
done 14618 <https://pharo.fogbugz.com/default.asp?14618> 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
Le 30/11/2014 12:01, Max Leske a écrit :
On 30.11.2014, at 11:18, Thierry Goubier <thierry.goubier@gmail.com>
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
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
On Aug 31, 2015, at 5:03 AM, Nicolai Hess <nicolaihess@web.de> wrote:
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 <nicolaihess@web.de>:
2014-11-30 20:54 GMT+01:00 stepharo <stepharo@free.fr>:
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 :
done 14618 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
Le 30/11/2014 12:01, Max Leske a écrit :
On 30.11.2014, at 11:18, Thierry Goubier <thierry.goubier@gmail.com> 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
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 and add 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 this rule: 3 have an explicit ifnonil check before accessing the variable - their implementation actually behaves correctly. The other two are just "testdata" used in the unit test for this rule 2015-08-31 15:30 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
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
On Aug 31, 2015, at 5:03 AM, Nicolai Hess <nicolaihess@web.de> wrote:
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 <nicolaihess@web.de>:
2014-11-30 20:54 GMT+01:00 stepharo <stepharo@free.fr>:
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 :
done 14618 <https://pharo.fogbugz.com/default.asp?14618> 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
Le 30/11/2014 12:01, Max Leske a écrit :
On 30.11.2014, at 11:18, Thierry Goubier <thierry.goubier@gmail.com>
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
On Tue, Sep 1, 2015 at 1:29 PM, Nicolai Hess <nicolaihess@web.de> wrote:
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 and add a nil check in the unwind block?
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.
And maybe we should improve that rule, because in Pharo 5.0 there are 5 methods catched by this rule: 3 have an explicit ifnonil check before accessing the variable - their implementation actually behaves correctly. The other two are just "testdata" used in the unit test for this rule
2015-08-31 15:30 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
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
On Aug 31, 2015, at 5:03 AM, Nicolai Hess <nicolaihess@web.de> wrote:
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 <nicolaihess@web.de>:
2014-11-30 20:54 GMT+01:00 stepharo <stepharo@free.fr>:
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 :
done 14618 <https://pharo.fogbugz.com/default.asp?14618> 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
Le 30/11/2014 12:01, Max Leske a écrit :
On 30.11.2014, at 11:18, Thierry Goubier <thierry.goubier@gmail.com>
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
-- _,,,^..^,,,_ best, Eliot
*Now* I understand this rule. 2015-09-02 3:40 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
On Tue, Sep 1, 2015 at 1:29 PM, Nicolai Hess <nicolaihess@web.de> wrote:
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 and add a nil check in the unwind block?
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.
And maybe we should improve that rule, because in Pharo 5.0 there are 5 methods catched by this rule: 3 have an explicit ifnonil check before accessing the variable - their implementation actually behaves correctly. The other two are just "testdata" used in the unit test for this rule
2015-08-31 15:30 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
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
On Aug 31, 2015, at 5:03 AM, Nicolai Hess <nicolaihess@web.de> wrote:
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 <nicolaihess@web.de>:
2014-11-30 20:54 GMT+01:00 stepharo <stepharo@free.fr>:
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 :
done 14618 <https://pharo.fogbugz.com/default.asp?14618> 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
Le 30/11/2014 12:01, Max Leske a écrit :
On 30.11.2014, at 11:18, Thierry Goubier <thierry.goubier@gmail.com>
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
-- _,,,^..^,,,_ best, Eliot
Sent from my iPhone
On Sep 7, 2015, at 2:16 PM, Nicolai Hess <nicolaihess@web.de> wrote:
*Now* I understand this rule.
Hi Nicolai, points to the need for some very good doc for these rules :-)
2015-09-02 3:40 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
On Tue, Sep 1, 2015 at 1:29 PM, Nicolai Hess <nicolaihess@web.de> wrote: 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 and add a nil check in the unwind block?
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.
And maybe we should improve that rule, because in Pharo 5.0 there are 5 methods catched by this rule: 3 have an explicit ifnonil check before accessing the variable - their implementation actually behaves correctly. The other two are just "testdata" used in the unit test for this rule
2015-08-31 15:30 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
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
On Aug 31, 2015, at 5:03 AM, Nicolai Hess <nicolaihess@web.de> wrote:
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 <nicolaihess@web.de>:
2014-11-30 20:54 GMT+01:00 stepharo <stepharo@free.fr>:
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 :
done 14618 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
Le 30/11/2014 12:01, Max Leske a écrit :
On 30.11.2014, at 11:18, Thierry Goubier <thierry.goubier@gmail.com> 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
-- _,,,^..^,,,_ best, Eliot
participants (7)
-
Eliot Miranda -
Max Leske -
Nicolai Hess -
stepharo -
Sven Van Caekenberghe -
Thierry Goubier -
Yuriy Tymchuk