[RFC Needed] better warnings on mutable struct#3968
Closed
dsyme wants to merge 209 commits intodotnet:dev15.6from
Closed
[RFC Needed] better warnings on mutable struct#3968dsyme wants to merge 209 commits intodotnet:dev15.6from
dsyme wants to merge 209 commits intodotnet:dev15.6from
Conversation
Contributor
|
Thanks Don - that’s a huge help when the optimiser is doing seemingly strange things that make complete sense when explained! |
Contributor
Author
|
@KevinRansom @brettfo The build-from-source is failing when an error message is added to Does the build-from-source rely on a checked-in version of a generated file? If so, can we remove that or have an ordinary build at least update the file for checkin? |
Contributor
Author
What's the process for automatically updating this file please? I tried |
Contributor
Author
|
CI seems broken - this is shown as "all checks passed" but no tests have been run |
dsyme
changed the title
Member
|
CI is down for all dotnet repos (us included) until at least 12/1, 3PM PST (~6 hours after I posted this.) |
Contributor
Author
|
Brett says:
It looks like we can do a manual update this way: |
dsyme
changed the title
dsyme
changed the title
* Enable pack for type providers * Add support to msbuild simplify to pack for typeprovider packages * Add transitive dependencies
…r-error-message Fix dotnet#2307.
* show type of ignored expression * fix tests * fix tests * fix test
* do not filter symbol uses by range length in getSymbolUsesInSolution because it causes loosing symbol occurrences in Inline Rename * make impl closer to the XAML one * remove obsolete import
* Update GetDeclarationListSymbols to add getAllEntities * Don't group types with the same display name as function overloads. Now that we can include all entities for top level completions, the grouping function can incorrectly group types with the same display name as function overloads. Use the compiled name instead as this includes the namespace. * Get logical name from ExnCase * Make getAllEntities optional Also renamed getAllSymbols to getAllEntities everywhere for consistency
* Enable net sdk project properties to be set in assembly info * Add back removed file * correct version * typo
* add entity_compiled_name, entity_other_range to EntityOptionalData * add entity_kind to EntityOptionalData * add entity_xmldoc, entity_xmldocsig to EntityOptionalData * add entity_tycon_abbrev to EntityOptionalData * add entity_tycon_repr_accessibility, entity_accessiblity to EntityOptionalData * add entity_exn_info to EntityOptionalData * remove XmlDocSif from TastPickle * fix unit check * add namespace check for unit
build portable PDBs from SDK projects and convert to Windows PDBs after build
* Found a workaround for the heavy code lens bug. Merged master into the branch. Signed-off-by: realvictorprm <[email protected]> * Fixes a deadlock due to context changes + started removing mutex. This is from yesterday! Review is NOT applied yet! Signed-off-by: realvictorprm <[email protected]> * Small fixes, applied a part of the reviews The UI is still buggy. This is very annoying. @saul I should report the bug you read about in my comments. . . Signed-off-by: realvictorprm <[email protected]> * Fixed CodeLens layout updates. Added possibility to enforce a re-layout of the visible code lens for the next layout-changed call. Applied suggestions from the reviews (could be that I missed one or two). Small cleanup. Small documentation additions. More checks. Removed some debug code. Added some debug code, most only appears if exceptions are thrown. The caching is still a small bit broken. At least I found out it's only a bit broken. Signed-off-by: realvictorprm <[email protected]> * Fixing removal of Code Lens. Reuse of elements still needs to be finished. I started it months ago but didn't finish it. Signed-off-by: realvictorprm <[email protected]> * Fixing fatal UI logic. Finally activated code for using cache results (so fixed a bug which included that code lens could be lost for us). Small cleanup to reduce confusion and decreased chance of introducing bugs due to this. Changing TaggerProvider to a more appropriate one. Discovered bugs in the visitor logic. * Added Line-Lens Added Options page to disable Code-Lens and to switch between Code-Lens and Line-Lens More caching fixes. Minimal UI fixes Signed-off-by: realvictorprm <[email protected]> * Fixed caching completely. Also fixed with this UI issues. Big cleanup. Now it's ready for another round of reviews! Signed-off-by: realvictorprm <[email protected]> * Added prefix option Signed-off-by: realvictorprm <[email protected]> * Fixing bug reported by Vasily Fixing cache bug. Fixing ui-offset bug. Fixing multi-view bug. Fixing build. Fixing signature bug reported by @saul Signed-off-by: realvictorprm <[email protected]> * Fixing KeyNotFoundException. Signed-off-by: realvictorprm <[email protected]> * Another UI fix. Code Lens are now hidden as soon as their line isn't visible anymore due to inserting new lines. Signed-off-by: realvictorprm <[email protected]> * Applied some review comments. Heavy performance improvements, now layouting isn't done every time due to that Visual Studio just transforms everything together (it's not viewport relative anymore, it's owner controlled but still in the wpf-text-view-coordinate-system)! The UI should now be fine in my opinion. It wasn't able for me to find a better workaround as just subtracting one pixel from the top position of the adornments. Signed-off-by: realvictorprm <[email protected]> * Another round of small cleanup. Signed-off-by: realvictorprm <[email protected]> * Another UI layout fix. Now weird code lens anymore :P Signed-off-by: realvictorprm <[email protected]> * Fixing merge * start working on color option * use darker gray à la normal CodeLens * Bind only from XAML. Fixes issue where "prefix" option was not saved * Undo unintended whitespace change * Should be part of previous commit * start working on color option * use darker gray à la normal CodeLens * Undo unintended whitespace change * Should be part of previous commit * fix value names * Applied review + small workaround for a bug. Merry Christmas! Signed-off-by: realvictorprm <[email protected]> * Removed old files * trying to fix CI Signed-off-by: realvictorprm <[email protected]> * First fix for member and constructor signatures! Signed-off-by: realvictorprm <[email protected]>
* Use C# code instead of manually modified DLL. * set /langversion:latest * pre-fill CSC_PIPE with Microsoft.Net.Compilers * upgrade Microsoft.Net.Compilers 2.4 => 2.7
…t#4547) * Revert "remove comment (dotnet#4538)" This reverts commit 4dce3d1. * Revert "Code Lens and Line Lens (dotnet#3608)" This reverts commit 8f8cbd6.
fix building in VS with PDB conversions
Improve perf of unused opens analysis
* forward warnings when searching for a method overload, fixes dotnet#3752 * add a new test for the bug * simplify test by using a `typecheck/sigs` tests * Update according to test * update test
* fixes for VisualFSharp.sln usage * fixes for VisualFSharp.sln usage * Update FSharpEmbedResourceText.fs * move MockTypeProviders * rename vsintegration/tests/unittests --> vsintegration/VisualFSharp.UnitTests * rename again * fix paths
Contributor
Author
|
Retarget at in master #4576 |
|
Would it be possible to have an attribute to control the defensive copy behaviour with an attribute? For example: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As described in #3923 in certain cases, the F# compiler assumes that .NET struct types are shallow-immutable when the user forgets to declare a struct value mutable using
let mutable x = ....This assumption is made because there is no way to analyze whether a .NET struct type is mutable, and
performing a defensive copy in situations like ``
This applies to method calls like
x.SetFoo(3)but not to property settersx.Foo <- 3. In the latter case the operation is assumed mutating and an error is given at compile time.This can lead to differences in optimized and non-optimized code when
let x = ....is used for a .NET struct value and a subsequent mutating operation is performed.The following adjustment is proposed for F# 4.x:
Give an optional warning (off by default) whenever a .NET struct type has been assumed to be immutable. However this is very noisy, as it triggers in very basic all property getter accessors are assumes to be immutable
Add a heuristic to give an improved diagnostic in some cases. Specifically if a method call has void/unit return type and starts with name "set" then give a warning (on by default) when used on a .NET struct value that has not been declared mutable.
Part (1) is fine, but part (2) of the change may be contentious: it is only a heuristic, but any name-based heuristic is unfortunate.
I'm slating this for F# 4.2 rather than pushing in the rule as a bug fix.