Update PDB behavior for 'using alias to type', and add EE and PDB tests. #67105
Conversation
| // We skip alias imports of embedded types to avoid breaking existing code that imports types | ||
| // that can't be embedded but doesn't use them anywhere else in the code. not, this is only | ||
| // done for named types. Other sorts of type symbols (arrays, etc.), are allowed through. | ||
| var typeRef = GetTypeReference((TypeSymbol)target, syntax, moduleBuilder, diagnostics); | ||
| usedNamespaces.Add(Cci.UsedNamespaceOrType.CreateType(typeRef, alias)); |
There was a problem hiding this comment.
note: there are def open questions on what the right behavior shoudl be here.
First, i'll note that prior to any of my changes here, this code was already potentially problematic wrt to instantiated named types (like List<EmbeddedType>). We would not skip those since List<> itself was not-linked. Unclear if that was desirable or not.
If that is not desirable, this needs to walk the entire symbol (including type arguments) to see if there are any embedded types.
If the above is intentional, and only the top-level construct is checked, then the new code follows the same logic. Namely that as all other types themselves are not-linked, though internally they may have a linked type (for example EmbeddedType[]), then it's ok to translate them along.
So, this code keeps similar semantics as before. We just need to decide if we want those semantics, or if we want to change things such that any usage of an embedded type would prevent translation.
tagging @AlekseyTs in case he has thoughts on this.
src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/DebuggerDisplayAttributeTests.cs
Show resolved
Hide resolved
CyrusNajmabadi
changed the title
CyrusNajmabadi
force-pushed
the
usingAliasTests
branch
from
de420d9
to
2f0e437
Compare
…to usingAliasTests
| } | ||
| } | ||
| "; | ||
| CompileAndVerify(text, options: TestOptions.DebugDll, targetFramework: TargetFramework.NetFramework).VerifyPdb(@" |
There was a problem hiding this comment.
yes. otherwise the referenced symbols come from other dlls, and the specific dlls are checked in the test output.
Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com>
Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com>
|
@tmat ptal. |
|
It looks like this was merged, not squashed |
|
Sorry. @jaredpar can you guys write a bot/rule that applies this project preference? I'm not sure what to do about it as github never seems to remember the setting, and so the standard (and preferable) behavior of merging PRs ends up happening, even if that's not what C# compiler wants :( i'm really not sure how else to get this to happen since the alternative is to depend on people, and people are squishy. |
Relates to test plan #56323