Skip to content

Conversation

@gusty
Copy link
Contributor

@gusty gusty commented Mar 2, 2021

This was discussed as part of fsharp/fslang-suggestions#691

@KevinRansom KevinRansom closed this Mar 3, 2021
@KevinRansom KevinRansom reopened this Mar 3, 2021
@gusty
Copy link
Contributor Author

gusty commented Mar 4, 2021

@KevinRansom I don't understand why I'm still getting this error stating that there's a difference in neg46 when I already removed the relevant part from the neg46.bsl

@cartermp
Copy link
Contributor

@gusty if you go here (for some reason not working on my mac laptop) and click on fsharpQA logs you can see detailed failures: https://dev.azure.com/dnceng/public/_build/results?buildId=1030986&view=artifacts&pathAsName=false&type=publishedArtifacts

@cartermp cartermp requested a review from dsyme March 11, 2021 02:12
@gusty gusty marked this pull request as ready for review March 11, 2021 07:27
@gusty
Copy link
Contributor Author

gusty commented Mar 11, 2021

So far here we're removing the limitation.

Another PR could adjust the warnings as suggested by @dsyme and remove it completely when the containing module is a rec one.

Please consider including this in the next preview as we need this in our projects to separate the business logic (at least in the same file at the top).

@Happypig375
Copy link
Member

Bump @dsyme

@KevinRansom KevinRansom self-assigned this Jun 28, 2021
@dsyme
Copy link
Contributor

dsyme commented Jun 29, 2021

@gusty The PR doesn't match what's approved?

The limitation ...[is to be]....turned into a warning when "namespace rec" is not used, and removed altogether when it is.

@gusty
Copy link
Contributor Author

gusty commented Jun 30, 2021

@dsyme as I said, it doesn't remove the warning in the rec case, so yes it doesn't match 100% but it fails (warns) on the safe side.

Anyway, I can have another look at it and try to suppress the warning in those cases, last time I checked I was unable to figure out where is that module information available. Any help would be much appreciated.

@dsyme
Copy link
Contributor

dsyme commented Jun 30, 2021

@gusty The PR doesn't add any tests showing what is now allowed

To be clearer, the approved suggestion is

  1. The warning message

    a.fs(8,14): warning FS0069: Interface implementations in augmentations are now deprecated. Interface implementations should be given on the initial declaration of a type
    

    gets changed to

    a.fs(8,14): warning FS0069:  Interface implementations should normally be given on the initial declaration of a type. Interface implementations in augmentations may lead to accessing static bindings before they are initialized, though only if the interface implementation is invoked during initialization of the static data, and in turn access the static data.  You may remove this warning using #nowarn "69" if you have checked this is not the case."
    

    I initially said it should be removed but I think it's more like warning 40 given its about about initialization soundness - something that can be optionally suppressed.

  2. The warning message is suppressed if namesapce rec is used

It's ok to just do part (1), since it;s only a warning message. Please also add tests for this as there will be very few or none in the codebase.

@T-Gro
Copy link
Member

T-Gro commented Oct 3, 2022

@gusty Hi, fine if I take this over ?

@T-Gro T-Gro self-assigned this Oct 3, 2022
@T-Gro T-Gro added this to the October-2022 milestone Oct 3, 2022
@gusty
Copy link
Contributor Author

gusty commented Oct 3, 2022

@T-Gro Sure, go ahead.

It's basically finished, just need to remove the warning in the mentioned case.
I didn't remove it as it requires some information about the module which is not directly available so have to figure out where can I pull it from.

@T-Gro
Copy link
Member

T-Gro commented Oct 6, 2022

@gusty ; @dsyme ; @vzarytovskii

Checks passing now, ready to be reviewed.

Copy link
Contributor Author

@gusty gusty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I can't approve as I'm considered as author. Thanks for taking care of this.

Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor nits. Looks great!

I'll mark as "comment" to not block approval - consider it approved besides these minor nits

@T-Gro T-Gro requested a review from dsyme October 10, 2022 08:19
0101
0101 previously approved these changes Oct 12, 2022
@T-Gro T-Gro requested review from 0101 and T-Gro October 12, 2022 13:08
@dsyme
Copy link
Contributor

dsyme commented Oct 17, 2022

@T-Gro I merged main into this

@dsyme
Copy link
Contributor

dsyme commented Oct 27, 2022

@vzarytovskii This could get mentioned in F# 7 release announcement, perhaps also a very brief RFC that the limitation has been lifted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

9 participants