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

Broken VariableLengthInlineArray<T> after upgrade from 0.3.49-beta to 0.3.106 #1205

Open
oold opened this issue Jun 10, 2024 · 8 comments
Open
Assignees
Labels
bug Something isn't working

Comments

@oold
Copy link

oold commented Jun 10, 2024

Actual behavior

Multiple build errors for VariableLengthInlineArray<T>:

Windows.Win32.VariableLengthInlineArray.g.cs(30,27,30,34): error CS8170: Struct members cannot return 'this' or other instance members by reference
Windows.Win32.VariableLengthInlineArray.g.cs(30,12,30,42): error CS8347: Cannot use a result of 'Unsafe.Add<T>(ref T, int)' in this context because it may expose variables referenced by parameter 'source' outside of their declaration scope

Expected behavior

The build succeeds.

Repro steps

  1. NativeMethods.txt content:
ApplicationDocumentLists
CopyFileEx
CoTaskMemFree
ExtractIconEx
FileOpenDialog
FileSaveDialog
GetActiveWindow
GetModuleFileName
GetSystemWindowsDirectory
HRESULT_FROM_WIN32
IApplicationDocumentLists
IEnumObjects
IFileDialog
IShellItem2
ITaskbarList3
SendMessage
SetActiveWindow
SHAddToRecentDocs
SHCreateItemFromParsingName
SHGetKnownFolderItem
SHOpenFolderAndSelectItems
SHParseDisplayName
SHSetTemporaryPropertyForItem
StrCmpLogical
TaskbarList
TaskDialogIndirect

E_*
FOLDERID_*
ICON_*
MAX_PATH
MESSAGEBOX_RESULT
PBST_*
PKEY_*
S_*
SHARD
TASKDIALOG_ELEMENTS
TASKDIALOG_MESSAGES
TD_*
WIN32_ERROR
WM_*
  1. NativeMethods.json content (if present):
  1. Any of your own code that should be shared?

Context

  • CsWin32 version: 0.3.106
  • Win32Metadata version (if explicitly set by project): 60.0.34-preview
  • Target Framework: net5.0-windows
@oold oold added the bug Something isn't working label Jun 10, 2024
@AArnott
Copy link
Member

AArnott commented Jun 12, 2024

I can't repro the problem. Can you provide a minimal repro project?

I couldn't repro even when targeting net5.0-windows as you mentioned. But FYI, we probably won't fix any bugs that are unique to that target framework, since the runtime itself is no longer supported by Microsoft.

@oold
Copy link
Author

oold commented Jun 13, 2024

cswin32-issue1205-repro.zip
The issue only occurs when building with Visual Studio 2019. I wasn't able to reproduce it in VS Code.

@AArnott
Copy link
Member

AArnott commented Jun 13, 2024

Oh! In that case I suspect the issue is the .NET SDK version you're compiling with. Can you type dotnet --list-sdks or otherwise tell me which SDK version you're using? VS2019 is a clue, but it isn't definitive.

@oold
Copy link
Author

oold commented Jun 13, 2024

> dotnet --list-sdks
3.1.426 [C:\Program Files\dotnet\sdk]
5.0.203 [C:\Program Files\dotnet\sdk]
5.0.214 [C:\Program Files\dotnet\sdk]
5.0.406 [C:\Program Files\dotnet\sdk]
5.0.408 [C:\Program Files\dotnet\sdk]
5.0.416 [C:\Program Files\dotnet\sdk]
6.0.203 [C:\Program Files\dotnet\sdk]
6.0.300 [C:\Program Files\dotnet\sdk]
6.0.407 [C:\Program Files\dotnet\sdk]
8.0.201 [C:\Program Files\dotnet\sdk]

MSBuild is using SDK version 6.0.203.

@AArnott
Copy link
Member

AArnott commented Jun 13, 2024

Great. I got it by using the 6.0.423 SDK and C# lang version 9. Investigating further...

@AArnott AArnott self-assigned this Jun 13, 2024
@AArnott
Copy link
Member

AArnott commented Jun 13, 2024

The C# language version isn't what does it. It's just the compiler version. The compiler that ships in .NET SDK 6.0.x fails where .NET SDK 8's compiler works. I guess the older compiler didn't know how to respect the [UnscopedRef] attribute. I suspect the only workaround we can make in the source generator will be to avoid emitting the VariableLengthInlineArray<T> type for that version of the compiler. That's trouble though, because the source generator isn't told what the compiler version is, I think. I'll have to get the roslyn team's take on it.

@jaredpar
Copy link
Member

The Unsafe.Add is a bit of a red herring here, from a ref safety standpoint the method looks like this:

internal ref T this[int index]
{
    [UnscopedRef]
    get => ref this.e0;
}

I guess the older compiler didn't know how to respect the [UnscopedRef] attribute

That is correct. The attribute was added in .NET 7 so the .NET 6 compiler doesn't attach any special meaning to it. From the perspective of .NET 6 compiler this method has no attribute and is a ref safety violation.

That's trouble though, because the source generator isn't told what the compiler version is, I think. I'll have to get the roslyn team's take on it.

Think it would be better to look at the language version than compiler version. That is available through parse options on the source files. In this case the language version is 9.0 which doesn't support ref fields, [UnscopedRef], etc ... That should be enough to know when this pattern is / isn't safe to code gen.

Note: in November the .NET 6 SDK goes out of support and at that time all supported compilers will be able to use these attributes. This pattern might be tricky to emit in older compilers.

@Nuklon
Copy link

Nuklon commented Jun 24, 2024

With .NET Framework this also causes problems. If you don't include the latest System.Runtime.CompilerServices.Unsafe package it fails to build with:

Error (active) CS0117 'Unsafe' does not contain a definition for 'SkipInit'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants