[PUP-4047] change wording of 'non productive expression' errors Created: 2015/02/26  Updated: 2015/11/20  Resolved: 2015/03/26

Status: Closed
Project: Puppet
Component/s: Compiler
Affects Version/s: None
Fix Version/s: PUP 3.7.5

Type: Improvement Priority: Normal
Reporter: Henrik Lindberg Assignee: Unassigned
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Template:
Epic Link: Puppet 4 Language
Story Points: 1
Sprint: Language 2015-03-04, Language 2015-03-18
QA Contact: Kurt Wall

 Description   

The new valiidator emits errors for non productive expressions. This because they are meaningless (produce values that are thrown away and does not cause side effects) and probably mask other problems.

While this is good in general - there are cases when developing code that makes this annoying.

Consider this:

if true {
  # commented out
}
$a = 10

Here, the entire if-expression is non productive (and it can be removed), It seems harsh to report this as an error (at all times).

I am reluctant to always turning this into a warning, but can imagine a mode where non-productive expressions are either ignored, or that they produce a warning. We could add a setting for 'non_productive_expression' and accept the values: ignore, warning, error - where error is the default.



 Comments   
Comment by Henrik Lindberg [ 2015/02/26 ]

ping James Sweeny

Comment by James Sweeny [ 2015/02/26 ]

Thanks for opening this Henrik Lindberg. In my opinion, we shouldn't even warn in cases like the example above, since there's absolutely nothing functionally wrong with the code from a user's perspective. If it's a style concern, I would rather this be in a linter than squawking during every compile (similar to having variables within single quotes).

That said, there are probably other examples you can think of where this kind of warning/error would actually prevent something unexpected from happening, so it might just be something we have to get more specific about.

Comment by Henrik Lindberg [ 2015/02/26 ]

Yes, you are right, one example is that space between an expression/variable and [] now is significant.

$a [3]

This leads to a non productive error for $a, since the logic above means, "produce the value of $a", then "produce an array with the value 3 in it".

The non productive error helps users that have written code that way. It would otherwise go undetected.

Comment by James Sweeny [ 2015/02/26 ]

In cases like that, would it make more sense to warn that there's a space between the variable and the array operator? If you Google "non-productive (construct|expression)" the only results are puppet-related, so it's not a very user-friendly warning.

Comment by Henrik Lindberg [ 2015/02/27 ]

Can not really warn on the space because it is ambiguous - it would issue false positives.

Agree that "non productive" is a not a very self explanatory phrase. Open to suggestions.

We considered "empty-expression", "expression without effect", (it is not "meaningless" - we do understand it), "vacuous expression", "void expression" (but associates more to expression not resulting in a value, which is not the case here, there may be a value, but that value is immediately thrown away), "discarded value expression", "unproductive", "useless", "worthless", "unworthwhile", "unreasonable" (as in 'you cannot possibly mean that...').

Tyler Pace Nicholas Fagerlund - any thoughts on this?

Comment by Greg Sarjeant [ 2015/02/27 ]

Would "expression has no effect" work, or is there a distinction between that and a non-productive expression that makes that wording inappropriate? That phrasing is a bit more self-explanatory, and googling it returns results from other languages which would help people to frame their debugging ("Oh, strange, that code should have done something, I wonder what's causing ... oops, I bet it's that space between the variable and the bracket."), if it's not obvious from the message and the line number.

Edit: This is very similar to "expression without effect," the main distinction is that anecdotally, the google results for "expression has no effect" seem like they'd be more helpful to someone trying to investigate.

Comment by Henrik Lindberg [ 2015/02/27 ]

"Expression has no effect" works. (Will wait a while for additional comments, but I am inclined to just change to that).

Comment by Henrik Lindberg [ 2015/02/27 ]

Just changed the two error messages to this:

    "This #{label.label(semantic)} has no effect. A value producing expression without other effect may only be placed last in a block/sequence"
    "This #{label.label(semantic)} has no effect. #{label.a_an_uc(container)} can not end with a just a value producing expression"

The first is the general case, the second when placing a value expression last in something like a class or define - e.g.

define foo() {
 # ...
 3
}

Can change them again if someone comes up with better ideas.

Comment by Tyler Pace [ 2015/02/27 ]

I agree with Greg Sarjeant recommendation. 'has no effect' is wonderfully self-explanatory and also sidesteps issuing any particular judgments about the expression in question.

I'm probably exposing my own lack of knowledge here, but how would this message be presented to the user? Is it an 'Error' or a 'Warning'? I don't know if Puppet 4 makes those distinctions, but if it does then I would suggest 'Warning'. Error implies that the code they just tried to compile is more or less non-functional, but I can see a lot of situations where users are actively debugging or fleshing out their code and have empty 'if' statements, functions with no logic, etc. This is the stuff that a user would typically check via a linter and might safely ignore depending on the context of their work, but because it's possible for some non productive expressions in Puppet 4 to have an impact beyond poor syntax or style then we probably shouldn't rely on optional linting to surface the warning.

Comment by Henrik Lindberg [ 2015/02/27 ]

Puppet does make a distinction between warnings and errors, we can change the errors to warnings easily (and it is also easy to make them configurable).
In the past we have erred on the side of being too permissive and people keep asking about having a strict mode. Being relaxed leads to sloppy, buggy programs.

While I agree that this is a "linting" kind of thing, turning off this validation, has the unfortunate side effect that gibberish programs will be accepted as valid:

[1,2,3]
[4,5,6]
1 2 3
ooops

would be a valid Puppet Program if the validations are removed.

Before we had this validation, we found that we made mistakes/typos that were not caught and it was difficult to spot the error (typically punctuation missing somewhere, a whitespace in the wrong place, a missing brace/bracket (while still having them balanced), etc.).

Now, should we treat some expressions that have no effect differently? I am not so sure that is a good idea. The one possibly valid use case so far is the "commenting out the body of an if when debugging" case - I would argue that you can just as well comment out the entire if expression (note that if the test expression has effect; assigns to variable, makes a call or invokes any other expression that may changes system state, then, conservatively the if expression is considered to have effect, also, if there is an else part and it has effect, the "then part" can be empty).

I think the very small cost of commenting out an extra line or two is a price worth paying since having the validation raise an error is likely to catch real mistakes in areas that those that are the least familiar with the language will make (and have a hard time finding the cause of), and will catch typos for the rest of us without having to search logs, turn on debugging flags etc. to find those problems.

Comment by Henrik Lindberg [ 2015/03/02 ]

Rephrased error messages - merged to stable and master.

Comment by Tyler Pace [ 2015/03/02 ]

I agree with your latest reasoning. There's very little extra work that needs to be done from the user to avoid messages about expected non-productive expressions and the cost of missing a warning about an unexpected and potentially problematic non-productive expression could be high.

Comment by Nicholas Fagerlund [ 2015/03/02 ]

PR or merge commit links?

Comment by Henrik Lindberg [ 2015/03/02 ]

commit: https://github.com/puppetlabs/puppet/commit/cc9ec6bd1cb47314fa8c408c9ad56f9aa2131020

Comment by Eric Thompson [ 2015/03/05 ]

QA risk assessment: medium
probability: medium
severity: low
test layer prediction: unit
evaluate unit tests. update testrail case 63972

Comment by Eric Thompson [ 2015/03/09 ]

verified on rhel7 at SHA: 6d8b65e

[root@j3nxa0euhmqwmrl puppet]# puppet apply non-productive.pp
Error: Could not parse for environment production: This 'if' statement has no effect. A value-producing expression without other effect may only be placed last in a block/sequence at /opt/puppet-git-repos/puppet/non-productive.pp:1:1 on node j3nxa0euhmqwmrl.delivery.puppetlabs.net
Error: Could not parse for environment production: This 'if' statement has no effect. A value-producing expression without other effect may only be placed last in a block/sequence at /opt/puppet-git-repos/puppet/non-productive.pp:1:1 on node j3nxa0euhmqwmrl.delivery.puppetlabs.net
[root@j3nxa0euhmqwmrl puppet]# cat non-productive.pp
if true {
  # commented out
}
$a = 10
 
 
[root@j3nxa0euhmqwmrl puppet]# puppet apply -e '$a [3]'
Error: Could not parse for environment production: This Variable has no effect. A value-producing expression without other effect may only be placed last in a block/sequence at line 1:1 on node j3nxa0euhmqwmrl.delivery.puppetlabs.net
Error: Could not parse for environment production: This Variable has no effect. A value-producing expression without other effect may only be placed last in a block/sequence at line 1:1 on node j3nxa0euhmqwmrl.delivery.puppetlabs.net
 
[root@j3nxa0euhmqwmrl puppet]# cat non-productive.pp
define foo() {
 # ...
 3
}
[root@j3nxa0euhmqwmrl puppet]# puppet apply non-productive.pp
Error: Could not parse for environment production: This Literal Integer has no effect. A 'define' expression can not end with a value-producing expression without other effect at /opt/puppet-git-repos/puppet/non-productive.pp:3:2 on node j3nxa0euhmqwmrl.delivery.puppetlabs.net
 
[root@j3nxa0euhmqwmrl puppet]# puppet apply -e '1 2 3'
Error: Could not parse for environment production: This Literal Integer has no effect. A value-producing expression without other effect may only be placed last in a block/sequence at line 1:1 on node j3nxa0euhmqwmrl.delivery.puppetlabs.net
Error: Could not parse for environment production: This Literal Integer has no effect. A value-producing expression without other effect may only be placed last in a block/sequence at line 1:1 on node j3nxa0euhmqwmrl.delivery.puppetlabs.net
[root@j3nxa0euhmqwmrl puppet]# puppet apply -e 'oops'
Notice: Compiled catalog for j3nxa0euhmqwmrl.delivery.puppetlabs.net in environment production in 0.37 seconds
Notice: Applied catalog in 0.01 seconds

Comment by Eric Thompson [ 2015/03/10 ]

we should probably fixup this ticket's title and description to match end result.
comments Henrik Lindberg?

Comment by Dan Bode [ 2015/11/20 ]

Using Puppet version 4.3.0, I am seeing that this fix does not work if the conditional statement is inside the scope of a class:

the code:

class foo (
  $var = true
) {
 
  if $var {
    # do some stuff...
  }
 
}
include foo

results in the following error:

Error: Could not parse for environment production: This 'if' statement has no effect. A Host Class Definition can not end with a value-producing expression without other effect at /private/tmp/foo.pp:5:3 on node danslaptop-2.local

However, if I remove the class definition:

$var = true
if $var {
  # do some stuff...
}

It compiles without error.

Comment by Henrik Lindberg [ 2015/11/20 ]

Dan Bode, you found a different problem. Please separate it out into a new ticket. You should be allowed to have an if expression last if that if is productive.

e.g. this is still not allowed

class foo {
  if true {
    3
  }
}

The reason it works without a class, is that "file scope code" may produce a value

Generated at Sun Apr 21 17:03:40 PDT 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.