Linter warning BCP334 is not correct when using several string functions.

[Gordonby]

Bicep version
0.22.6

Describe the bug
I am receiving a BCP334 warning for a case that cannot occur. I am starting the name with the static value of stflow.

Warning BCP334: The provided value can have a length as small as 0 and may be too short to assign to a target with a configured minimum length of 3.

To Reproduce

var flowLogStorageName = take(replace(toLower('stflow${resourceName}${uniqueString(resourceGroup().id, resourceName)}'),'-',''),24)
resource flowLogStor 'Microsoft.Storage/storageAccounts@2023-01-01' = {
  name: flowLogStorageName
  kind: 'StorageV2'
  sku: {
    name: 'Standard_LRS'
  }
  location: location
  properties: {
    minimumTlsVersion: 'TLS1_2'
  }
}

Additional context
Add any other context about the problem here.

[jeskew]

There are two workarounds you can use:

1. You could disable this diagnostic:

   var flowLogStorageName = take(replace(toLower('stflow${resourceName}${uniqueString(resourceGroup().id, resourceName)}'),'-',''),24)
   resource flowLogStor 'Microsoft.Storage/storageAccounts@2023-01-01' = {
     #disable-next-line BCP334
     name: flowLogStorageName
     kind: 'StorageV2'
     sku: {
       name: 'Standard_LRS'
     }
     location: location
     properties: {
       minimumTlsVersion: 'TLS1_2'
     }
   }

2. Or you can reorder the functions in the expression a bit:

   var flowLogStorageName = take(toLower('stflow${replace(resourceName, '-', '')}${uniqueString(resourceGroup().id, resourceName)}'),24)

Fixing the analysis here will be a bit tricky. Because the value of resourceGroup().id can't be calculated at compile time, toLower('stflow${resourceName}${uniqueString(resourceGroup().id, resourceName)}') is represented in the type system as a string with at least 19 characters. Since replace doesn't know the exact value of the first argument, it's assuming that the string could be entirely composed of '-' characters.

The type system currently only knows about string literals (like 'stflow') or strings with optional min/max lengths; there could be a separate string type introduced that is aware that is has literal segments.

[StephenWeatherford]

Sounds like by design to me. The message is a warning and does say "could be". The proper fix it is the disable-next-line.

[abatishchev]

This warning breaks my ADO build because it's uses BuildQualityChecks to ensure no new warnings. I could suppress this one in one way or another, but I'd like it not to be emitted in the first place. Here's my file:

param dcResourceGroupName string
param clusterPrefix string
param stamp string
param environment string
param location string

var azureRegionLookup = {
  westus: {
    azureRegionAbbreviation: 'wus'
  }
  westus2: {
    azureRegionAbbreviation: 'wus2'
  }
  westus3: {
    azureRegionAbbreviation: 'wus3'
  }
}
var storageAccountName = '${clusterPrefix}${stamp}${environment}${azureRegionLookup[location].azureRegionAbbreviation}os'

resource storage 'Microsoft.Storage/storageAccounts@2023-04-01' existing = {
  name: storageAccountName
  scope: resourceGroup(dcResourceGroupName)
}

Proposal: since the warning says "may" and my resource is marked as existing, should the warning be not emitted?

[jeskew]

@abatishchev The type system could be updated to detect that azureRegionLookup[location].azureRegionAbbreviation is 'wus' | 'wus2' | 'wus3' | error, though I expect that will be complex to implement. As a workaround, would an empty string really be an acceptable clusterPrefix, stamp, or environment? Adding @minLength(1) to any one of those declarations would ensure that the storage account name is at least three characters.

[abatishchev]

Adding @minlength(1)

Sure, this makes sense and is the simplest one, would clearly hint the validation system that the sum length is >3

But what about this one:

Proposal: since the warning says "may" and my resource is marked as existing, should the warning be not emitted?

Why to run validation on existing resources in the first place? If it exists, then it's valid by definition.

[jeskew]

Why to run validation on existing resources in the first place? If it exists, then it's valid by definition.

True, but the deployment would still fail if it tried to query a storage account with a two-letter name (which is what the existing declaration will cause ARM to do). This could be incorrect if an RP's validation rule was added after the existing resource was created, and the resource was grandfathered in. E.g., if Storage were to up the minimum length to 4 characters for new accounts, they would probably not force users with 3-character account names to recreate them.

That seems fairly niche, though, so I'd still like to see more upvotes before we commit to this.

[abatishchev]

True, but the deployment would still fail if it tried to query a storage account with a two-letter name

reference() would fail with a simple 404, because the resource doesn't exist because it cannot. The call would fail at ARM, won't even reach the RP so it won't have a chance to run the validation.