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

Update shared code from linker #70204

Merged
merged 23 commits into from
Jun 7, 2022
Merged

Conversation

vitek-karas
Copy link
Member

This brings in relatively recent version of the shared code from the dotnet/linker repo.
Main changes:

  • ValueNode -> SingleValue - and the partial classes implementation for the values
  • Adapt MethodBodyScanner to the SingleValue/MultiValue
    • Includes some fixes around array handling and unknown values
  • Removal of ReflectionPatternContext
    • Partially replaced by DiagnosticContext
    • Partially removed (the checks that everything reports something are nto needed anymore)
  • Use HandleCallAction and RequireDynamicallyAccessedMembersAction - this replaces most of the functionality in ReflectionMethodBodyScanner.

Formatting:
The files which are shared exactly from linker are kept as is (so tabs and so on)
The files which are AOT specific should follow the formatting of AOT projects

Testing:
Passes smoke tests and some additional validation done via linker tests which is not part of this change

Note: This is not up-to-date with linker but it's getting us much closer. Known "TODOs":

  • CompilerGeneratedState - linker has a much newer and much more capable version. Port of that will come later (as it was in heavy development while I was doing this port, and it's not strictly necessary for current functionality)
  • Type hierarchy marking - this needs work on the linker side as it's not yet part of the shared codebase
  • Correctly handle RUC/RDC for all "reflection accesses" to methods - some of the change improve on this, but it's nowhere near where it needs to be. Future work item.

@vitek-karas
Copy link
Member Author

Some changes made to the linker sources to help with this are here: dotnet/linker#2818

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@@ -1,5 +1,5 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// Copyright (c) .NET Foundation and contributors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Just checking to make sure we really want to touch the licensing header on this. This repo pretty much universally uses the header this is replacing.

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'll update the AOT files (like this one to use the existing header).
The files which come from linker I would like to keep exactly identical - and linker uses the other header.

I'm not clear why there are two different headers (the one in linker was provided by LCA relatively recently, so maybe it's more "up to date"?).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I definitely don't want us to introduce extra diffs compared to the dotnet/linker repo.

@stephentoub I've seen you doing these copyright header changes in the past - do you know what is it with the various repos using various headers? Looks like Copyright (c) .NET Foundation and contributors is also used in the dotnet/sdk repo (besides dotnet/linker).

This change is introducing files shared between dotnet/linker repo and dotnet/runtime repo and we have a licensing header difference.

{
missingMemberTypesString = string.Empty;

var missingMemberTypes = GetMissingMemberTypes (targetMemberTypes, sourceMemberTypes);
Copy link
Member

Choose a reason for hiding this comment

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

I would rather we just let VS reformat these files to the coding style followed by this repo so that we don't need to context switch the coding style based on the directory the file is in. Having vastly different coding style per directory is confusing to anyone who doesn't have full context.

We don't run a code formatter script in this repo because we rely on people just following common sense. I only very rarely comment on style issues. It mostly just works out.

We weren't really able to follow the odd coding standards in the dotnet/linker repo until Marek introduced the CI gestapo to enforce it.

If you prefer to keep it this way, please at least add an .editorconfig that stomps over the settings at the root of the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was to keep the files which come from linker repo exactly the same files - so that updating them is easy and all diffs are clearly visible. Also, updates to these files should ideally be done in the linker repo first (to keep one primary source).

So personally I would prefer we keep these files formatted as per dotnet/linker rules for now (and update the .editorconfig). I plan to move linker to runtime repo once we branch for .NET 8 and then we would reformat everything to match the runtime style (... can't wait ...).

But that's just my opinion, if you think it's more important to match the runtime style I'll change it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's likely the odd code style choices from dotnet/linker repo are going to interfere with various repo-wide cleanups people sometimes do (using tabs instead of spaces is the main one that I can see this directory fighting with). Just raising this.

Use the runtime's license header on the files which are AOT's own (shared files still keep the header from linker for now).

Added editorconfig to keep the formatting of the shared files the same as in linker.
@vitek-karas vitek-karas merged commit 174c23e into dotnet:main Jun 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 7, 2022
@vitek-karas vitek-karas deleted the SyncToLinker branch September 26, 2023 09:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants