bicep icon indicating copy to clipboard operation
bicep copied to clipboard

`no-loc-expr-outside-params` rule hides errors

Open afscrome opened this issue 3 years ago • 6 comments

Bicep version VS Code v0.6.18

Describe the bug The no-loc-expr-outside-params analyzer will fire on expressions even if they're invalid in the current scope. (e.g. resourceGroup().location in a subscription scope). This expression is then highlighted as a warning rather than an error, hiding the underlying error

To Reproduce

In the below example, the no-loc-expr-outside-param reports a warning, however Iw ouldn't expect the warnign to be logged at all as the entire expression is invalid in the current (subscription) scope.

targetScope='subscription'

resource rg 'Microsoft.Resources/resourceGroups@2021-04-01' = {
  location: resourceGroup().location  
}

image

Additional context Add any other context about the problem here.

afscrome avatar May 26 '22 08:05 afscrome

I don't think the error is hidden -- I see a red squiggle under the parens (). Here is what the error looks like w/ the warnings disabled:

image

Maybe the span for this error should cover both the function name and the parens?

alex-frankel avatar May 31 '22 19:05 alex-frankel

Now you point it out, I can see it, but I'd still argue it was hidden as the warning dominated it. I even logged #6961 around the same time and didn't notice the error squiggly in this issue.

Fixing #6961 would definitely help as it would make the error clearer, but I still don't think the warning should be firing at all in this scenario.

In this scenario resourceGroup().location is not a location value - it is an invalid expression roughly equilivant to any(null).location which doesn't cause the warning .

On a similar note, the warning doesn't fire in the following situation which I would expect it to complain about

targetScope='resourceGroup'
var foo = resourceGroup()
var bar = foo.location

It looks to me like the rule is using a text match rather than a symbolic match. If it were using a symbolic match, then it should catch the second scenario, and not fire on the first as I'd expect.

afscrome avatar May 31 '22 19:05 afscrome

Curious what @ucheNkadiCode and @StephenWeatherford think about whether we should throw a warning in this case. It makes sense to me that we may not want to if this expression is also going to cause an error.

alex-frankel avatar Jun 01 '22 20:06 alex-frankel

I agree we shouldn't show the warning for an invalid expression. The resourceGroup() location property is only available for resource group scope, correct?

StephenWeatherford avatar Jun 01 '22 23:06 StephenWeatherford

I like that hierarchy of Error (and only the error) first, then show a warning once that error is fixed, then all goes away!

Thanks @afscrome

ucheNkadiCode avatar Jun 02 '22 19:06 ucheNkadiCode

Proposal: I'll fix the rule to only fire if resourceGroup().location is valid for the current scope, then consider this bug fixed. @afscrome good with that?

StephenWeatherford avatar Jun 02 '22 19:06 StephenWeatherford