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

[Lint] Double force casting #217

Open
jacob-tucker opened this issue Sep 27, 2023 · 7 comments
Open

[Lint] Double force casting #217

jacob-tucker opened this issue Sep 27, 2023 · 7 comments
Labels
Feedback Good First Issue Improvement Technical work without new features, refactoring, improving tests

Comments

@jacob-tucker
Copy link

Current Behavior

Right now if you double force cast, for example, test["testing"]!!, the linter will tell you that you are performing an unecessary force unwrap.

However, given the fact that it's possible for dictionaries to have nil values, this actually may be needed some times to convert, for example, a String?? to a String

Expected Behavior

The linter should not complain about a double force cast if the type is a double optional

Steps To Reproduce

pub fun main(): AnyStruct {
  let bin: {String: String} = { "gg": "10" }
  var v : {String: AnyStruct} =  { "deniz": bin["gg"] }
  let thing = v["deniz"]!!.getType()
  return thing
}

Environment

- Cadence version: 0.40.0
- Network: Emulator (or any)
@jacob-tucker jacob-tucker added Bug The issue represents a bug, malfunction, regression Feedback labels Sep 27, 2023
@turbolent turbolent changed the title [Linter] <Double force casting> [Linter] Double force casting Sep 27, 2023
@turbolent turbolent changed the title [Linter] Double force casting [Lint] Double force casting Sep 27, 2023
@turbolent
Copy link
Member

Doesn't this work as expected:

  • The value type of v is AnyStruct
  • v["deniz"] results in AnyStruct?
  • Two force-unwraps are used, but only one is necessary to unwrap

@bluesign
Copy link
Contributor

I think problem is: AnyStruct covering optional types. This is working as intended ( warning ) for sure.

But I think this should be only allowed to written as :

pub fun main(): AnyStruct {
  let bin: {String: String} = { "gg": "10" }
  var v : {String: AnyStruct?} =  { "deniz": bin["gg"] }
  let thing = v["deniz"]!!.getType()
  return thing
}

But as AnyStruct covers AnyStruct? this is going into area of cannot be statically determined.

@turbolent turbolent added Improvement Technical work without new features, refactoring, improving tests and removed Bug The issue represents a bug, malfunction, regression labels Sep 27, 2023
@turbolent
Copy link
Member

Yeah, that's what I tried to point out: The analysis works only on static types, and from that perspective the implementation is correct.

However, you're right that AnyStruct as the top-type covers any other dynamic type, so the dynamic type of the underlying value could actually be optional. This is unlike any other type (e.g. think through the example with the dictionary's value type being e.g. Int).

So the analysis could be improved here to ignore Any/AnyStruct/AnyResouce, to avoid reporting a warning that the operator is unnecessary, where it might actually be necessary. 👍

@turbolent
Copy link
Member

turbolent commented Sep 27, 2023

In particular, the exception should be added here:

_, ok = valueType.(*sema.OptionalType)
if !ok {

@bluesign

This comment was marked as off-topic.

@turbolent

This comment was marked as off-topic.

@turbolent

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feedback Good First Issue Improvement Technical work without new features, refactoring, improving tests
Projects
None yet
Development

No branches or pull requests

3 participants