[yast-devel] Ruby 'next' considered dangerous
We just had to deal with a P1 bug caused by a stray 'next' statement in the Ruby code that did not return a value: https://bugzilla.suse.com/show_bug.cgi?id=955216#c11 We seem to have a large number of such 'next' statements in our code. Everybody please be aware that this can cause great trouble in certain iterator methods that expect a return value from their code block! In this case, it was Hash::reduce(). If you simply call 'next' in its loop, it will merrily clobber its accumulator with 'nil', very likely causing a crash in the next loop iteration. IMHO it's sad that we do have tools that enforce stuff like one newline too many between function definitions, but critical things like not returning a value from a block that is required to return one goes completely unnoticed. And we were even lucky that our QA caught this problem, not some unsuspecting customer in the field. What can we do to improve this? I suggest some Ruby experts to collect all such functions that are potentially in danger of this problem and then grep through the code where they are used, and if any similar 'next' without a required return value is used. But that's just a one-time effort. Maybe there is some tool out there that can automate that for us. Kind regard -- Stefan Hundhammer <shundhammer@suse.de> YaST Developer SUSE Linux GmbH GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg) Maxfeldstr. 5, 90409 Nürnberg, Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, 17 Nov 2015 15:01:30 +0100 Stefan Hundhammer <shundhammer@suse.de> wrote:
We just had to deal with a P1 bug caused by a stray 'next' statement in the Ruby code that did not return a value:
https://bugzilla.suse.com/show_bug.cgi?id=955216#c11
We seem to have a large number of such 'next' statements in our code. Everybody please be aware that this can cause great trouble in certain iterator methods that expect a return value from their code block!
In this case, it was Hash::reduce(). If you simply call 'next' in its loop, it will merrily clobber its accumulator with 'nil', very likely causing a crash in the next loop iteration.
IMHO it's sad that we do have tools that enforce stuff like one newline too many between function definitions, but critical things like not returning a value from a block that is required to return one goes completely unnoticed. And we were even lucky that our QA caught this problem, not some unsuspecting customer in the field.
What can we do to improve this?
I suggest some Ruby experts to collect all such functions that are potentially in danger of this problem and then grep through the code where they are used, and if any similar 'next' without a required return value is used. But that's just a one-time effort. Maybe there is some tool out there that can automate that for us.
Kind regard
Hi, well, there is already quite powerful parsers and I believe it won't be so hard to improve rubocop to check it. On other hand it is exactly same problem as using return without value. So for me it is same as ``` def recursive return unless funny_condition if a == 1 return obj else return obj.recursive end end ``` And to be honest it can happen in any code in any language ( just depending how often it can happen ). E.g. in ycp one nil can result in cascade of nils. What is ruby philosophy how to solve it? Tests. Do not trust code that is not tested. Of course some tools can detect some misusage, but only good tests can prove that code really works. Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 18.11.2015 09:56, Josef Reidinger wrote:
And to be honest it can happen in any code in any language ( just depending how often it can happen ).
Wrong. Any halfway decent parser will realize that in one case you do return something from your function, and in another you don't. That's what happened here. Better yet, if you have to DECLARE your stuff (functions, variables), you are making a commitment (or a contract, however you want to name this) what you are going to do with that thing and what you don't. And the parser (no matter if it's an interpreter or a compiler) can catch those problems.
E.g. in ycp one nil can result in cascade of nils.
I really wouldn't call YCP, of all things, a shining example of good behaviour when it comes to this. Yet, even it had (after years of painful debugging) static type checking that at least did some minimal amount of plausibility checks.
What is ruby philosophy how to solve it? Tests. Do not trust code that is not tested. Of course some tools can detect some misusage, but only good tests can prove that code really works.
Tests are good, but they have limits. In particular, when your number of code paths is too high, the possible permutations just explode. That's why achieving good test coverage is so difficult. Ruby (and the tools in its ecosystem like Rubocop) try to make you believe that you can easily handle this by dumbing down functions enough so they become trivial. Well, that would be good - if only there were a practical way to go through each code path at least once (better yet, with some permutations of your data set / variables to test fringe cases in the less frequently taken code paths, too). But as seen in this example, this is often not practical. If your code depends on the status of things beyond your immediate control, you'd have to mock all that. In this particular case, who would have foreseen that a seemingly unrelated scenario like having a RAID on the previous installation might lead to such a crash? Sure, better test coverage might have caught this. But it would have been even simpler (very much simpler!) to have the parser throw an error if the code is inconsistent - like not returning a value in this case if in the other case it does. And to cut the reply that immediately comes to mind short: If in some pathological case I want the code to return 'nil', I can explicitly make it return 'nil'. IMHO there is no real excuse for the Ruby parser to behave this dumb. They just became collateral damage of their own "anything goes" philosophy. CU -- Stefan Hundhammer <shundhammer@suse.de> YaST Developer SUSE Linux GmbH GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg) Maxfeldstr. 5, 90409 Nürnberg, Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Wed, 18 Nov 2015 10:13:45 +0100 Stefan Hundhammer <shundhammer@suse.de> wrote:
On 18.11.2015 09:56, Josef Reidinger wrote:
And to be honest it can happen in any code in any language ( just depending how often it can happen ).
Wrong.
Any halfway decent parser will realize that in one case you do return something from your function, and in another you don't. That's what happened here.
Preciselly speak you always return something. Just maybe default value for return is not so common in other languages. I know that e.g. C++ can check it during compilation, but ruby is not compiled, so in the end, you in both cases ends up with runtime error or error catched by test suite.
Better yet, if you have to DECLARE your stuff (functions, variables), you are making a commitment (or a contract, however you want to name this) what you are going to do with that thing and what you don't. And the parser (no matter if it's an interpreter or a compiler) can catch those problems.
This is discussion between static and dynamic languages and about explicit types specification. I think there is enough discussion on internet regarding this stuff and I worry I cannot add anything new here.
E.g. in ycp one nil can result in cascade of nils.
I really wouldn't call YCP, of all things, a shining example of good behaviour when it comes to this. Yet, even it had (after years of painful debugging) static type checking that at least did some minimal amount of plausibility checks.
What is ruby philosophy how to solve it? Tests. Do not trust code that is not tested. Of course some tools can detect some misusage, but only good tests can prove that code really works.
Tests are good, but they have limits. In particular, when your number of code paths is too high, the possible permutations just explode. That's why achieving good test coverage is so difficult.
Thats more phylosophy discussion about egg or the egg problem. Ruby phylosophy is that if something is not tested ( manually or automatic ), then you cannot believe in code. And with good code design almost everything can be tested.
Ruby (and the tools in its ecosystem like Rubocop) try to make you believe that you can easily handle this by dumbing down functions enough so they become trivial. Well, that would be good - if only there were a practical way to go through each code path at least once (better yet, with some permutations of your data set / variables to test fringe cases in the less frequently taken code paths, too).
well, trivial = single purpose. Actually what you mention already exists for ruby - https://github.com/mbj/mutant it tests if you do not have fake test coverage ( martin played with it in past ). Regarding automatic tool to go thrue all path, it is still not enough as 1) that tool need to know behavior, sometimes exception is correct 2) need to verify it do what is expected, just passing function without exception is not enough
But as seen in this example, this is often not practical. If your code depends on the status of things beyond your immediate control, you'd have to mock all that. In this particular case, who would have foreseen that a seemingly unrelated scenario like having a RAID on the previous installation might lead to such a crash?
If it happen first time like in this case I usually capture target map ( as hand crafting target map is pain ) and then write at least regression test, that I know that covering this case.
Sure, better test coverage might have caught this. But it would have been even simpler (very much simpler!) to have the parser throw an error if the code is inconsistent - like not returning a value in this case if in the other case it does.
It is question if you are on side at least have some check or having good tests that verify working solution. Again it is part of mandatory types discussion topic.
And to cut the reply that immediately comes to mind short: If in some pathological case I want the code to return 'nil', I can explicitly make it return 'nil'.
On other hand there is many cases where nil is nature return code like in each, find, etc. So for language authors it can mean to use specific statement for other loops or be consistent, which can cause troubles.
IMHO there is no real excuse for the Ruby parser to behave this dumb. They just became collateral damage of their own "anything goes" philosophy.
I agree that faster failure during runtime will be better, but there can be maybe code that use this code path. Josef
CU
-- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
I forgot to mention two things... 1) it is not just "next", "break" is also affected. E.g. try using break in reduce, it also produce nil. Or even more funny irb(main):001:0> [0, 1, 2].map { |a| break 15 } => 15 2) recommended way at least in recent ruby is to use each_with_object instead of reduce ( http://apidock.com/rails/Enumerable/each_with_object ) which do not use return value, but modify passed object. Example code irb(main):002:0> [0, 1, 2].each_with_object({}) { |a, res| a == 1 ? next : res[a] = a.to_s } => {0=>"0", 2=>"2"} On Wed, 18 Nov 2015 11:33:23 +0100 Josef Reidinger <jreidinger@suse.cz> wrote:
On Wed, 18 Nov 2015 10:13:45 +0100 Stefan Hundhammer <shundhammer@suse.de> wrote:
On 18.11.2015 09:56, Josef Reidinger wrote:
And to be honest it can happen in any code in any language ( just depending how often it can happen ).
Wrong.
Any halfway decent parser will realize that in one case you do return something from your function, and in another you don't. That's what happened here.
Preciselly speak you always return something. Just maybe default value for return is not so common in other languages. I know that e.g. C++ can check it during compilation, but ruby is not compiled, so in the end, you in both cases ends up with runtime error or error catched by test suite.
Better yet, if you have to DECLARE your stuff (functions, variables), you are making a commitment (or a contract, however you want to name this) what you are going to do with that thing and what you don't. And the parser (no matter if it's an interpreter or a compiler) can catch those problems.
This is discussion between static and dynamic languages and about explicit types specification. I think there is enough discussion on internet regarding this stuff and I worry I cannot add anything new here.
E.g. in ycp one nil can result in cascade of nils.
I really wouldn't call YCP, of all things, a shining example of good behaviour when it comes to this. Yet, even it had (after years of painful debugging) static type checking that at least did some minimal amount of plausibility checks.
What is ruby philosophy how to solve it? Tests. Do not trust code that is not tested. Of course some tools can detect some misusage, but only good tests can prove that code really works.
Tests are good, but they have limits. In particular, when your number of code paths is too high, the possible permutations just explode. That's why achieving good test coverage is so difficult.
Thats more phylosophy discussion about egg or the egg problem. Ruby phylosophy is that if something is not tested ( manually or automatic ), then you cannot believe in code. And with good code design almost everything can be tested.
Ruby (and the tools in its ecosystem like Rubocop) try to make you believe that you can easily handle this by dumbing down functions enough so they become trivial. Well, that would be good - if only there were a practical way to go through each code path at least once (better yet, with some permutations of your data set / variables to test fringe cases in the less frequently taken code paths, too).
well, trivial = single purpose. Actually what you mention already exists for ruby - https://github.com/mbj/mutant it tests if you do not have fake test coverage ( martin played with it in past ). Regarding automatic tool to go thrue all path, it is still not enough as 1) that tool need to know behavior, sometimes exception is correct 2) need to verify it do what is expected, just passing function without exception is not enough
But as seen in this example, this is often not practical. If your code depends on the status of things beyond your immediate control, you'd have to mock all that. In this particular case, who would have foreseen that a seemingly unrelated scenario like having a RAID on the previous installation might lead to such a crash?
If it happen first time like in this case I usually capture target map ( as hand crafting target map is pain ) and then write at least regression test, that I know that covering this case.
Sure, better test coverage might have caught this. But it would have been even simpler (very much simpler!) to have the parser throw an error if the code is inconsistent - like not returning a value in this case if in the other case it does.
It is question if you are on side at least have some check or having good tests that verify working solution. Again it is part of mandatory types discussion topic.
And to cut the reply that immediately comes to mind short: If in some pathological case I want the code to return 'nil', I can explicitly make it return 'nil'.
On other hand there is many cases where nil is nature return code like in each, find, etc. So for language authors it can mean to use specific statement for other loops or be consistent, which can cause troubles.
IMHO there is no real excuse for the Ruby parser to behave this dumb. They just became collateral damage of their own "anything goes" philosophy.
I agree that faster failure during runtime will be better, but there can be maybe code that use this code path.
Josef
CU
-- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Wed, Nov 18, 2015 at 11:33:23AM +0100, Josef Reidinger wrote:
But as seen in this example, this is often not practical. If your code depends on the status of things beyond your immediate control, you'd have to mock all that. In this particular case, who would have foreseen that a seemingly unrelated scenario like having a RAID on the previous installation might lead to such a crash?
If it happen first time like in this case I usually capture target map ( as hand crafting target map is pain ) and then write at least regression test, that I know that covering this case.
Well, the patch for bug #930091 which introduced the wrong 'next' statement (and btw. nothing else) was apparently not tested, neither with unit-tests nor with integration tests. I'm sure there are good reasons for that, e.g. time pressure. But it shows that the concept of 'we have to test everything' does not work in practice. So any help from static code analysis tools should be welcomed or to quote John Carmack "it is irresponsible to not use it" [1]. Regards, Arvin [1] http://www.gamasutra.com/view/news/128836/InDepth_Static_Code_Analysis.php -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 18.11.2015 12:32, Arvin Schnell wrote:
But it shows that the concept of 'we have to test everything' does not work in practice. So any help from static code analysis tools should be welcomed or to quote John Carmack "it is irresponsible to not use it" [1].
Looks like there are a lot of tools that do purely formal stuff like Rubocop, i.e. enforcing if your blanks and empty lines are set correctly, but not very many that concentrate on technical issues. One I just found with a 5 minute Google search was this: https://github.com/YorickPeterse/ruby-lint Has anybody here tried that before? Maybe it's useful for us. Kind regards -- Stefan Hundhammer <shundhammer@suse.de> YaST Developer SUSE Linux GmbH GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg) Maxfeldstr. 5, 90409 Nürnberg, Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Wed, 18 Nov 2015 13:23:31 +0100 Stefan Hundhammer <shundhammer@suse.de> wrote:
On 18.11.2015 12:32, Arvin Schnell wrote:
But it shows that the concept of 'we have to test everything' does not work in practice. So any help from static code analysis tools should be welcomed or to quote John Carmack "it is irresponsible to not use it" [1].
Looks like there are a lot of tools that do purely formal stuff like Rubocop, i.e. enforcing if your blanks and empty lines are set correctly, but not very many that concentrate on technical issues.
One I just found with a 5 minute Google search was this:
https://github.com/YorickPeterse/ruby-lint
Has anybody here tried that before? Maybe it's useful for us.
Kind regards
I don't try it, but looks interesting. I just do not think we should make it "hard" requirement (like do not submit package when something found). More like hints what can go wrong as for ruby with meta programming only think you can say for sure, is that everything can be changed :) Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 18.11.2015 13:33, Josef Reidinger wrote:
One I just found with a 5 minute Google search was this:
https://github.com/YorickPeterse/ruby-lint
Has anybody here tried that before? Maybe it's useful for us.
I don't try it, but looks interesting. I just do not think we should make it "hard" requirement (like do not submit package when something found). More like hints what can go wrong as for ruby with meta programming only think you can say for sure, is that everything can be changed :)
Looks like a good idea for HackWeek: December 7th to 11th. Bye Lukas -- Lukas Ocilka, Systems Management (Yast) Team Leader SLE Department, SUSE Linux -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Wed, Nov 18, 2015 at 01:39:16PM +0100, Lukas Ocilka wrote:
On 18.11.2015 13:33, Josef Reidinger wrote:
One I just found with a 5 minute Google search was this:
https://github.com/YorickPeterse/ruby-lint
Has anybody here tried that before? Maybe it's useful for us.
I don't try it, but looks interesting. I just do not think we should make it "hard" requirement (like do not submit package when something found). More like hints what can go wrong as for ruby with meta programming only think you can say for sure, is that everything can be changed :)
Looks like a good idea for HackWeek: December 7th to 11th.
Thanks for the pointer! I've had a short look at ruby-lint. It seems it would be a bit of work to get it working on our code because it needs to know all the classes referenced in the code. So I ran it on ruby-dbus which is non-trivial but almost self contained, and ruby-lint did find a bug already: $ ruby-lint lib/dbus/message_queue.rb message_queue.rb: warning: line 37, column 14: unused local variable d message_queue.rb: warning: line 37, column 17: unused local variable d message_queue.rb: warning: line 64, column 49: unused argument m (^ these unused vars are also detected by RuboCop) message_queue.rb: error: line 95, column 61: undefined instance variable @path message_queue.rb: error: line 100, column 39: undefined instance variable @path (^ but this bug is not) On the other hand, it also complains type.rb: error: line 201, column 1: undefined method module_function which is an elementary method in Module. -- Martin Vidner, YaST Team http://en.opensuse.org/User:Mvidner Kuracke oddeleni v restauraci je jako fekalni oddeleni v bazenu
On Tue, Nov 17, 2015 at 03:01:30PM +0100, Stefan Hundhammer wrote:
We just had to deal with a P1 bug caused by a stray 'next' statement in the Ruby code that did not return a value:
https://bugzilla.suse.com/show_bug.cgi?id=955216#c11
[...] Maybe there is some tool out there that can automate that for us.
((77c4c57...)) mvidner@mrakoplas:yast-bootloader$ ~/svn/rubocop/bin/rubocop --only Lint/ReduceNextNil Inspecting 63 files ....................................W.......................... Offenses: src/modules/BootStorage.rb:158:9: W: Lint/ReduceNextNil: Use next with an accumulator argument in a reduce. next unless partitions ^^^^ 63 files inspected, 1 offense detected WIP in https://github.com/bbatsov/rubocop/compare/master...mvidner:reduce-next-nil?... -- Martin Vidner, YaST Team http://en.opensuse.org/User:Mvidner Kuracke oddeleni v restauraci je jako fekalni oddeleni v bazenu
On Thu, 3 Dec 2015 16:01:00 +0100 Martin Vidner <mvidner@suse.cz> wrote:
On Tue, Nov 17, 2015 at 03:01:30PM +0100, Stefan Hundhammer wrote:
We just had to deal with a P1 bug caused by a stray 'next' statement in the Ruby code that did not return a value:
https://bugzilla.suse.com/show_bug.cgi?id=955216#c11
[...] Maybe there is some tool out there that can automate that for us.
((77c4c57...)) mvidner@mrakoplas:yast-bootloader$ ~/svn/rubocop/bin/rubocop --only Lint/ReduceNextNil Inspecting 63 files ....................................W..........................
Offenses:
src/modules/BootStorage.rb:158:9: W: Lint/ReduceNextNil: Use next with an accumulator argument in a reduce. next unless partitions ^^^^
63 files inspected, 1 offense detected
WIP in https://github.com/bbatsov/rubocop/compare/master...mvidner:reduce-next-nil?...
Nice, I think it can be easily accepted upstream. BTW only think I do not like is skipping after first instance found. Better to not skip subsequent offenders. Josef
participants (5)
-
Arvin Schnell -
Josef Reidinger -
Lukas Ocilka -
Martin Vidner -
Stefan Hundhammer