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-mismatched [<DefaultParameterValue>] is not ignored enough #18314

Open
IS4Code opened this issue Feb 13, 2025 · 5 comments · May be fixed by #18330
Open

Type-mismatched [<DefaultParameterValue>] is not ignored enough #18314

IS4Code opened this issue Feb 13, 2025 · 5 comments · May be fixed by #18330
Assignees
Milestone

Comments

@IS4Code
Copy link

IS4Code commented Feb 13, 2025

There is an inconsistency in how an incorrect placement of [<DefaultParameterValue>] is handled. Usually, in most situations, the parameter is simply not treated as optional at all, but if it is of type obj, it seems it is still considered optional.

Repro steps

open System.Runtime.InteropServices

type C =
  static member Foo([<Optional>][<DefaultParameterValue("hello")>] obj: obj) =
    printfn "%A" obj

C.Foo()

Expected behavior

C.Foo() should fail to compile, since the attribute has mismatched type and thus even [<Optional>] is ignored.

Actual behavior

The code compiles and passes null to Foo, in contrary to the warning:

FS3211 The default value does not have the same type as the argument. The DefaultParameterValue attribute and any Optional attribute will be ignored. Note: 'null' needs to be annotated with the correct type, e.g. 'DefaultParameterValue(null:obj)'.

Known workarounds

Providing a different type than obj (as far as I can tell) correctly stops the code from being compilable at all.

Related information

Environment: https://sharplab.io/#v2:DYLgZgzgPg9gDgUwHYAIDKBPCAXBBbAOgCUBXJbASzwQIElyEAneNJgNwoGMEIBYAKAHYMiFAGEUAXgEoUOAIaVOKangBGTFADEYMABQBtADwB5OJRhJ5wAHwBdYwBEEYeSWDYACvMbzquRgA1axIEPQAiAAsEYGAYcIBKexQYNQArEBT0hKkZWRQ4RgpyMFRwgFIAQXCstIEBMQIdfQSgA=

@T-Gro
Copy link
Member

T-Gro commented Feb 14, 2025

It should behave as if using any other mismatched type annotation, indeed.

@T-Gro
Copy link
Member

T-Gro commented Feb 19, 2025

Figured out what is happening - the method is indeed typechecked as one without optional arguments, but the C.Foo() call is interpreted as passing Unit to C.Foo. Which satisfies being an obj, and is emited as null at runtime.

I will check if there are some places which intentionally relly on having a Unit passed, cannot imagine any right now.

@T-Gro
Copy link
Member

T-Gro commented Feb 19, 2025

If you remove the attributes, it stops producing the FS3211 warning, and the code still runs - treating () as an argument which fits obj.

@IS4Code
Copy link
Author

IS4Code commented Feb 19, 2025

Ah, the optional parameter was a red herring then. Thank you for investigating this!

Do you know of a way that would make C.Foo() (or some other call syntax) fail to compile in this case? The call now makes sense to me based on F# rules, but it might be confusing and could cause bugs when overloads are changed. I suppose with --checknulls+ there would be at least a warning, but still with obj | null one might not want to include unit into the set of accepted argument types.

Otherwise we can close this.

@T-Gro
Copy link
Member

T-Gro commented Feb 19, 2025

I will check if we couldn't expand the warnings on implicit conversions.
In this case, the conversion is from Unit to obj.

I am certain there will be hypothetical use cases for which a full error would be a breaking change, but I believe a warning should be acceptable.

@T-Gro T-Gro linked a pull request Feb 20, 2025 that will close this issue
@T-Gro T-Gro linked a pull request Feb 20, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: New
Development

Successfully merging a pull request may close this issue.

2 participants