Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type checking is not working for intermediate variables #691

Closed
anthony-c-martin opened this issue Oct 20, 2020 · 5 comments · Fixed by #2444
Closed

Type checking is not working for intermediate variables #691

anthony-c-martin opened this issue Oct 20, 2020 · 5 comments · Fixed by #2444
Assignees
Labels
bug Something isn't working type system
Milestone

Comments

@anthony-c-martin
Copy link
Member

anthony-c-martin commented Oct 20, 2020

Repro here:

resource vmWorking 'Microsoft.Compute/virtualMachines@2020-06-01' = {
  name: 'working'
  location: 'west us'
  properties: {
    // diagnostics raised - we have a property that doesn't make sense.
    valThatDoesNotExist: ''
  }
}

var vmNotWorkingProps = {
  valThatDoesNotExist: ''
}

resource vmNotWorking 'Microsoft.Compute/virtualMachines@2020-06-01' = {
  name: 'notWorking'
  location: 'west us'
  // no diagnostics raised here even though the type is invalid!
  properties: vmNotWorkingProps
}
@ghost ghost added the Needs: Triage 🔍 label Oct 20, 2020
@anthony-c-martin anthony-c-martin added bug Something isn't working and removed Needs: Triage 🔍 labels Oct 20, 2020
@majastrz
Copy link
Member

Yeah, that's a bad bug. Definitely need to fix that in 0.2.

@alex-frankel alex-frankel added this to the v0.2 milestone Oct 20, 2020
@anthony-c-martin
Copy link
Member Author

Did some digging on this; it's actually not going to be that simple to fix. In type validation we check for various syntaxes to recurse over:

// object assignability check

We need to ensure we recurse over VariableAccessSyntax, but need to be careful to attach diagnostics to the reference to the variable reference, rather than the variable value itself.

@anthony-c-martin
Copy link
Member Author

We should make sure when we fix this that for the following:

var varWithLiterals = [
  {
    prop: 'ValA'
  }
  {
    prop: 'ValB'
  }
]

Our type system should be able to infer the type of:

varWithLiterals[someIndex].prop

as 'ValA' | 'ValB' (union of string literals) rather than just string.

@anthony-c-martin
Copy link
Member Author

I'm planning to discuss this with the team today - we have some decisions to make about what the errors will look like.

@anthony-c-martin
Copy link
Member Author

Notes from discussion:

  • See if we can provide a path to the offending property.
  • The error/warning should be shown on the property value, not the property key.
  • Each mismatched property should be handled as a separate error/warning unless the prefix for the log is really long.
  • We should investigate what newlines in errors/warnings looks like in VSCode hovers & problems window to see if that's viable.
  • There are going to be a lot of edge cases. We should start small (e.g. by validating 1 property deep), and expand if necessary.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working type system
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants