Skip to content

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Sep 26, 2022

Corresponding spec change: dotnet/csharplang#6492
Corresponding proposal: dotnet/csharplang#6476

Note: from follow-up discussion with Chuck and David, we need to keep the stackalloc scenario an error, as the runtime doesn't support it.

@jcouv jcouv self-assigned this Sep 26, 2022
@ghost ghost added the Area-Compilers label Sep 26, 2022
@jcouv jcouv changed the base branch from main to release/dev17.4 September 27, 2022 18:43
@jcouv
Copy link
Member Author

jcouv commented Sep 27, 2022

/azp run


In reply to: 1259913235

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 27, 2022

Azure Pipelines successfully started running 4 pipeline(s).

In reply to: 1259913936

@jcouv jcouv marked this pull request as ready for review September 27, 2022 18:47
@jcouv jcouv requested a review from a team as a code owner September 27, 2022 18:47
@jcouv jcouv requested review from RikkiGibson and cston September 27, 2022 23:27
@jcouv jcouv added this to the 17.4 milestone Sep 28, 2022
@RikkiGibson RikkiGibson self-assigned this Sep 28, 2022
diagnostics.Add(ErrorCode.ERR_ManagedAddr, location, type);
return true;
}
diagnostics.Add(ErrorCode.WRN_ManagedAddr, location, type);
Copy link
Member

@RikkiGibson RikkiGibson Sep 28, 2022

Choose a reason for hiding this comment

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

We should also do a LangVersion check so that people on old language versions can't suppress these #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's a good idea. I'll do a runtime rebuild tonight to verify they don't have a project with managed pointers and LangVer<11.

Copy link
Member Author

Choose a reason for hiding this comment

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

Following conversation with Jared, I added a LangVer check. Thanks

Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM aside from the LangVersion concern.

diagnostics.Add(ErrorCode.WRN_ManagedAddr, location, type);
}

return false;
Copy link
Contributor

@cston cston Sep 29, 2022

Choose a reason for hiding this comment

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

return false

Should this return true if the language version check fails? #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

If not, the method comment should be updated to reflect the new behavior.

@cston
Copy link
Contributor

cston commented Sep 29, 2022

C.M<StructWithRefField>(); // 3

Should this be StructWithIndirectRefField or StructWithRefField<int>?


In reply to: 1262856400


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:8675 in 39a1e43. [](commit_id = 39a1e43, deletion_comment = False)

Julien Couvreur added 2 commits September 29, 2022 15:01
@cston
Copy link
Contributor

cston commented Sep 29, 2022

    C* y3 = stackalloc C[0]; // 4

From offline discussion, it sounds like these stackalloc cases should remain as errors.


In reply to: 1262902999


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:1482 in 39a1e43. [](commit_id = 39a1e43, deletion_comment = False)

@cston
Copy link
Contributor

cston commented Sep 29, 2022

// Dev10 and Roslyn produce the same diagnostic here.

Perhaps "Dev10 and Roslyn produce diagnostics here."


In reply to: 1262923374


In reply to: 1262923374


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/UnsafeTests.cs:8387 in 39a1e43. [](commit_id = 39a1e43, deletion_comment = False)

@azure-pipelines
Copy link

Command 'run

In' is not supported by Azure Pipelines.



Supported commands

  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@jcouv
Copy link
Member Author

jcouv commented Sep 29, 2022

// Dev10 and Roslyn produce the same diagnostic here.

This is just a comment in an existing test. I'll leave as-is


In reply to: 1262923374


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/UnsafeTests.cs:8387 in 39a1e43. [](commit_id = 39a1e43, deletion_comment = False)

@jcouv jcouv enabled auto-merge (squash) September 30, 2022 00:38
@jcouv jcouv merged commit 23ecea2 into dotnet:release/dev17.4 Sep 30, 2022
@jcouv jcouv deleted the unsafe-warnings3 branch September 30, 2022 02:34
jaredpar added a commit to jaredpar/runtime that referenced this pull request Sep 30, 2022
dotnet/roslyn#64294

Compiler now issues warnings for pointer operations involving managed
types
jaredpar added a commit to jaredpar/runtime that referenced this pull request Sep 30, 2022
dotnet/roslyn#64294

Compiler now issues warnings for pointer operations involving managed
types
jaredpar added a commit to dotnet/runtime that referenced this pull request Oct 3, 2022
* Patches for scoped locals

dotnet/roslyn#64093

This change enforced that `scoped` on a local set the escape scope to
the current block where previously it was incorrectly setting to the
containing method.

* Make return and out equivalent for ref safety

dotnet/roslyn#64318

This change allows anything returnable from a method to be assigned to
an `out` parameter. In several places had to add `scoped` to `ref` to
inform compiler they could not be captured in an `out` parameter.

* Warnings on managed pointer types

dotnet/roslyn#64294

Compiler now issues warnings for pointer operations involving managed
types

* Update compiler version

* PR feedback

* Ref safety rules attribute

Co-authored-by: Charles Stoner <[email protected]>
carlossanlop pushed a commit to dotnet/runtime that referenced this pull request Oct 4, 2022
* Patches for scoped locals

dotnet/roslyn#64093

This change enforced that `scoped` on a local set the escape scope to
the current block where previously it was incorrectly setting to the
containing method.

* Make return and out equivalent for ref safety

dotnet/roslyn#64318

This change allows anything returnable from a method to be assigned to
an `out` parameter. In several places had to add `scoped` to `ref` to
inform compiler they could not be captured in an `out` parameter.

* Warnings on managed pointer types

dotnet/roslyn#64294

Compiler now issues warnings for pointer operations involving managed
types

* Update compiler version

* Fixup

* Ref safety rules attribute

Co-authored-by: Charles Stoner <[email protected]>
@kasperk81
Copy link
Contributor

@jcouv in the following code, X1 warns twice, while X2 warns just once for the right hand side. is this expected?

using System;
using System.Runtime.InteropServices;

[StructLayout(LayoutKind.Sequential)]
public ref struct StackAllocatedByRefs
{
    internal ref byte arg;
}

public class C
{
    public void X1()
    {
        StackAllocatedByRefs byrefStorage = default;
        unsafe
        {
            StackAllocatedByRefs* x = &byrefStorage;
        }
    }

    public void X2()
    {
        StackAllocatedByRefs byrefStorage = default;
        unsafe
        {
            var x = &byrefStorage;
        }
    }
}

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants