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

FS-1093 op_Implicit 3395 warnings missing for constructors and potentially inconsistent #13437

Open
NinoFloris opened this issue Jul 4, 2022 · 4 comments
Labels
Area-Compiler-Checking Type checking, attributes and all aspects of logic checking Bug Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code.
Milestone

Comments

@NinoFloris
Copy link
Contributor

NinoFloris commented Jul 4, 2022

Take the following example. Compiled with <OtherFlags>$(OtherFlags) --warnon:3395</OtherFlags> it produces no warnings on the constructor.

open System

// Minimized copy for demonstration of Microsoft.Extensions.Primitives.StringValues
type StringValues(v: string) =
    static member op_Implicit(values: StringValues): string = ""
    
type Foo(_v: Nullable<int>) =
    member val Test: Nullable<int> = 0 with get,set // Warning for 3391 as I would have interpreted it.
    member val StringValue: string = "" with get,set
    
    static member Create() =
      let value = 1
      let instance = Foo(value, Test = value) // No warning on either the argument or the property setter?
      instance.Test <- value // No warning for 3395 and 3391?
      instance.StringValue <- StringValues("1") // Warning for 3395
      
      let mutable test: Nullable<int> = value // Warning for 3391 as I would have interpreted it.
      
      // Warning for 3391 but it's syntactically identical to the line that had no warning and one that had 3395???
      test <- 1 
@NinoFloris NinoFloris added the Bug label Jul 4, 2022
@charlesroddie
Copy link
Contributor

I was trying to add these warning yesterday and ran into this. The number has changed to 3395!

I fixed the RFC here: fsharp/fslang-design#686

I also couldn't find documentation on the xml syntax for --warnon but <WarnOn>FS3389; FS3395</WarnOn> seems to work.

@NinoFloris
Copy link
Contributor Author

@charlesroddie thanks! I'll update the issue as I still think it's somewhat inconsistent when it triggers for surface syntax.

As for the syntax you probably want <WarnOn>$(WarnOn); 3395</WarnOn> or old style <OtherFlags>$(OtherFlags) --warnon:3395</OtherFlags>

@NinoFloris NinoFloris changed the title FS-1093 op_Implicit 3390 warnings don't seem to work FS-1093 op_Implicit 3395 warnings are inconsistent Jul 5, 2022
@baronfel
Copy link
Member

baronfel commented Jul 5, 2022

For a bit of an explainer why you'd want that syntax: Charles' version overwrites those properties, so any modifications by any other part of the build would be lost. Nino's version persists the old value of the property, making the change additive overall.

@NinoFloris
Copy link
Contributor Author

I also didn't get the FS prefixed versions to work, additionally I just created this issue over at JetBrains/resharper-fsharp#379

@NinoFloris NinoFloris changed the title FS-1093 op_Implicit 3395 warnings are inconsistent FS-1093 op_Implicit 3395 warnings missing for constructors and potentially inconsistent Jul 5, 2022
@vzarytovskii vzarytovskii moved this to Not Planned in F# Compiler and Tooling Jul 7, 2022
@vzarytovskii vzarytovskii added this to the Backlog milestone Jul 7, 2022
@dsyme dsyme added the Area-Compiler-Checking Type checking, attributes and all aspects of logic checking label Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-Checking Type checking, attributes and all aspects of logic checking Bug Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code.
Projects
Status: New
Development

No branches or pull requests

5 participants