Skip to content

File types EE support#62812

Merged
RikkiGibson merged 30 commits intodotnet:mainfrom
RikkiGibson:file-types-ee
Sep 6, 2022
Merged

File types EE support#62812
RikkiGibson merged 30 commits intodotnet:mainfrom
RikkiGibson:file-types-ee

Conversation

@RikkiGibson
Copy link
Member

@RikkiGibson RikkiGibson commented Jul 21, 2022 •
edited
Loading

Closes #62334
Closes #62332

  • Use SHA256 checksum of the file path instead of an ordinal for the file type metadata name. This was already requested by IDE team, and we found it was useful to do this here because the PDB APIs are oriented around identifying documents by their name/content SHA, not by their ordinal in the document table.
  • Introduce a diagnostic when the containing file of a file type doesn't have a unique file path in the compilation. (we will need to update file-local-types.md to specify this)
  • Decode the file type name on metadata symbols in order to bind usages of them in EE scenarios

There are some TODOs in here, some of which represent open questions. Any input you have on them would be appreciated.

@cston @jcouv @tmat @dibarbet for review.

@ghost ghost added the Area-Compilers label Jul 21, 2022
@RikkiGibson RikkiGibson marked this pull request as ready for review July 21, 2022 03:33
@RikkiGibson RikkiGibson requested review from a team as code owners July 21, 2022 03:33
Youssef1313
Youssef1313 reviewed Jul 21, 2022
View reviewed changes
@RikkiGibson

This comment was marked as resolved.

Sign in to view
cston
cston reviewed Aug 8, 2022
View reviewed changes
cston
cston reviewed Aug 8, 2022
View reviewed changes
cston
cston reviewed Aug 8, 2022
View reviewed changes
cston
cston reviewed Aug 9, 2022
View reviewed changes
cston
cston reviewed Aug 9, 2022
View reviewed changes
@RikkiGibson RikkiGibson mentioned this pull request Aug 31, 2022
Open
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Aug 31, 2022

Done with review pass (commit 26)

@RikkiGibson RikkiGibson requested a review from AlekseyTs August 31, 2022 22:35
davidwengier
davidwengier approved these changes Aug 31, 2022
View reviewed changes
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

I can't see anything wrong with the IDE changes :D

AlekseyTs
AlekseyTs reviewed Sep 1, 2022
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/Source/FileModifierTests.cs
// file1.cs(5,9): error CS0103: The name 'C1' does not exist in the current context
// C1.M1();
Diagnostic(ErrorCode.ERR_NameNotInContext, "C1").WithArguments("C1").WithLocation(5, 9));
var retargeted = comp1.GetMember<NamedTypeSymbol>("C1");
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 1, 2022

Choose a reason for hiding this comment

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

retargeted

Consider asserting IsFileLocal value. I assume it should be false.

AlekseyTs
AlekseyTs approved these changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 28)

@RikkiGibson
Copy link
Member Author

RikkiGibson commented Sep 1, 2022 •
edited
Loading

Validation insertion (internal only): https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/421815 (edited)

@RikkiGibson
Copy link
Member Author

RikkiGibson commented Sep 6, 2022

CloudBuild and RPS are clean on the VS insertion so will merge.

@RikkiGibson RikkiGibson merged commit 1324e73 into dotnet:main Sep 6, 2022
@RikkiGibson RikkiGibson deleted the file-types-ee branch September 6, 2022 23:59
@ghost ghost added this to the Next milestone Sep 6, 2022
jaredpar
jaredpar reviewed Sep 7, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

namespace Microsoft.CodeAnalysis.CSharp;

internal struct FileIdentifier
Copy link
Member

@jaredpar jaredpar Sep 7, 2022

Choose a reason for hiding this comment

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

Shouldn't this be readonly?

jaredpar
jaredpar reviewed Sep 7, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs
try
{
var encodedFilePath = s_encoding.GetBytes(filePath);
using var sha256 = SHA256.Create();
Copy link
Member

@jaredpar jaredpar Sep 7, 2022

Choose a reason for hiding this comment

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

Need to avoid hard coding any crypto algorithms in our code base. Instead need to use one of our centralized locations. When we don't centralize it means we get hassled hard as crypto algorithms are deprecated and we have to find all the individual places it was used. Likely should centralize in SourceHashAgorithms

Copy link
Member Author

@RikkiGibson RikkiGibson Sep 7, 2022

Choose a reason for hiding this comment

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

I wasn't sure what the correct usage would look like here. It seems like SourceHashAlgorithms itself just has helpers for converting to/from the GUID used to denote a hash algorithm in a portable PDB.

@RikkiGibson RikkiGibson mentioned this pull request Sep 7, 2022
Closed
@allisonchou allisonchou mentioned this pull request Sep 11, 2022
Closed
@tmat tmat mentioned this pull request Sep 16, 2022
Open
@Cosifne Cosifne modified the milestones: Next, 17.4 P3 Sep 26, 2022
@jcouv jcouv mentioned this pull request Oct 7, 2022
Closed
83 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaredpar jaredpar jaredpar left review comments

@davidwengier davidwengier davidwengier approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

+2 more reviewers

@Youssef1313 Youssef1313 Youssef1313 left review comments

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Compilers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support file-local types in EE Define meaning of BuckStopsHereBinder.AssociatedSyntaxTree in EE scenarios

7 participants

@RikkiGibson @AlekseyTs @jaredpar @davidwengier @cston @Youssef1313 @Cosifne