Use enum for method parameter indexing #2993

jtschuster · 2022-08-22T22:19:38Z

I've run into multiple issues when I think an int parameterIndex is the source-based index when it is the IL-based index (offset by 1 for this) or vice versa.

In order to fix this, we can use different enums for each definition of "Parameter Index" to enforce the correct usage and avoid the off by one errors. The enums rely on the underlying int value to provide the index number.

This change adds ILParameterIndex to represent the IL-based indexing where 0 may refer to this, and SourceParameterIndex where 0 is the first "real" parameter in C# source code. Converting from ILParameterIndex to SourceParameterIndex returns a SourceParameterKind to differentiate between thisand other parameters and ensure that anySourceParameterIndexwill be able to indexMethodReference.Parameters`.

It uses these in ParameterReferenceValue as well as GetMethodParameterValue() and adds a GetMethodThisParameterValue(), ThisParameterReferenceValue, and bubbles the changes where necessary.

Most places should use SourceParameterIndex, and convert to ILParameterIndex when necessary or at the boundary of updated and old code.

I'll continue to update the code to use these enums in other PRs.

vitek-karas

I didn't look at the details, but I like the approach of using C# type system to differentiate the two cases - really nice idea!

vitek-karas · 2022-08-23T08:31:00Z

src/linker/Linker/ParameterHelpers.cs

+			}
+		}
+
+		public static SourceParameterIndex GetSourceParameter (MethodDefinition method, Instruction operation)


Suggested change

public static SourceParameterIndex GetSourceParameter (MethodDefinition method, Instruction operation)

public static SourceParameterIndex GetSourceParameterIndex (MethodDefinition method, Instruction operation)

vitek-karas · 2022-08-23T08:33:47Z

src/linker/Linker/ILParameterIndex.cs

+	/// <summary>
+	/// Represents the index of arguments passed to a function in IL. Generally prefer to use <see cref="ILLink.Shared.SourceParameterIndex"/> when possible. See also <seealso cref="Mono.Linker.ParameterHelpers"/>.
+	/// </summary>
+	/// 


Please add a comment describing that you're using enums as a way to separate source/IL indexes in the C# type system (took me a while to figure that out), and that they're actually used as numbers (relying on the fact that enum is really just an Int32).
Would also be nice to describe the numbering scheme here (IL parameter indexes are >=0 and for "instance" call the 0 is this and others are +1 .... and then something similar for source indexes).

vitek-karas · 2022-08-23T08:34:15Z

src/ILLink.Shared/SourceParameterIndex.cs

+{
+	/// <summary>
+	/// Used to indicate the index of the parameter in source code (i.e. the same regardless of if there is a `this` or not)
+	/// </summary>


Same comment as for ILParameterIndex

vitek-karas · 2022-08-23T08:35:51Z

src/ILLink.Shared/SourceParameterIndex.cs

+	/// <summary>
+	/// Used to indicate the index of the parameter in source code (i.e. the same regardless of if there is a `this` or not)
+	/// </summary>
+	public enum SourceParameterIndex


I find the naming a bit weird - it's not exactly clear what "source" means here. That said - I didn't come up with a better name really. So I guess we should just add a good comment (maybe add couple of examples).

sbomer

I like it, thank you!

sbomer · 2022-08-23T14:15:11Z

src/linker/Linker.Dataflow/MethodBodyScanner.cs


 			bool isByRef;

 			// Thank you Cecil, Operand being a ParameterDefinition instead of an integer,


This comment can move to ILParameterIndexFromInstruction 🤣

sbomer · 2022-08-23T14:15:50Z

src/linker/Linker.Dataflow/MethodBodyScanner.cs

-					paramNum = paramDefinition.Index;
-				}
-
-				// This is semantically wrong if it returns true - we would representing a reference parameter as a reference to a parameter - but it should be fine for now


This comment is still true right? Please keep it in the new code below.

src/linker/Linker/TypeMapInfo.cs

jtschuster · 2022-08-24T17:46:51Z

Sorry for the force push, I ran into issues with rebasing and it seemed like the cleanest option. I've updated the description to describe the updated code. This version differentiates SourceParameterIndex and this parameters to ensure the SourceParameterIndex will always be able to index MethodReference.Parameters

tlakollo

Only nit stuff from my review

tlakollo · 2022-08-24T17:48:52Z

src/linker/Linker/ParameterHelpers.cs

+		public static ILParameterIndex ILParameterIndexFromSourceParameterIndex (MethodReference method, SourceParameterIndex sourceIndex)
+		{
+			if (method.HasImplicitThis ())
+				return (ILParameterIndex) ((int)sourceIndex + 1);
+
+			return (ILParameterIndex) (int)sourceIndex;
+		}


Nit: maybe we can simplify it to

Suggested change

public static ILParameterIndex ILParameterIndexFromSourceParameterIndex (MethodReference method, SourceParameterIndex sourceIndex)

{

if (method.HasImplicitThis ())

return (ILParameterIndex) ((int)sourceIndex + 1);

return (ILParameterIndex) (int)sourceIndex;

}

public static ILParameterIndex ILParameterIndexFromSourceParameterIndex (MethodReference method, SourceParameterIndex sourceIndex) =>

method.HasImplicitThis () ? (ILParameterIndex) ((int)sourceIndex + 1) : (ILParameterIndex) (int)sourceIndex;

tlakollo · 2022-08-24T17:53:32Z

src/linker/Linker/ILParameterIndex.cs

+	/// Represents the index of arguments passed to a function in IL (i.e. (ILParameterIndexs)0 represents `this` for non-static methods.
+	/// This is used to enforce a differentiatiation between scenarios where the 0 index should be `this` and when the 0 index should be the first non-this parameter in the type system.
+	/// There are no named enum values, the underlying integer value represents the index value.


Suggested change

/// Represents the index of arguments passed to a function in IL (i.e. (ILParameterIndexs)0 represents `this` for non-static methods.

/// This is used to enforce a differentiatiation between scenarios where the 0 index should be `this` and when the 0 index should be the first non-this parameter in the type system.

/// There are no named enum values, the underlying integer value represents the index value.

/// Represents the index of arguments passed to a function in IL (i.e. (ILParameterIndex)0 represents `this` for non-static methods.

/// This is used to enforce a differentiation between scenarios where the 0 index should be `this` and when the 0 index should be the first non-this parameter in the type system.

/// There are no named enum values, the underlying integer value represents the index value.

tlakollo · 2022-08-24T17:55:29Z

src/linker/BannedSymbols.txt

+P:Mono.Collections.Generic.Collection`1{Mono.Cecil.ParameterDefinition}.Item(System.Int32); use x
+P:Mono.Cecil.ParameterDefinitionCollection.Item(System.Int32); use x


What's this BannedSymbols file?

Ah, I was looking into using the BannedApiAnalyzer to prevent indexing MethodReference.Parameters directly with an int, but I couldn't get it to work. These changes shouldn't be there.

tlakollo · 2022-08-24T17:56:50Z

src/ILLink.Shared/SourceParameterIndex.cs

+	/// This enum and <see cref="T:Mono.Linker.ILParameterIndex"/> is used to enforce a differentiatiation between scenarios where the 0 index should be `this` and when the 0 index should be the first non-this parameter in the type system.
+	/// `this` is the only named enum value. For all others, the underlying integer value represents the index value.


Suggested change

/// This enum and <see cref="T:Mono.Linker.ILParameterIndex"/> is used to enforce a differentiatiation between scenarios where the 0 index should be `this` and when the 0 index should be the first non-this parameter in the type system.

/// `this` is the only named enum value. For all others, the underlying integer value represents the index value.

/// This enum and <see cref="T:Mono.Linker.ILParameterIndex"/> is used to enforce a differentiation between scenarios where the 0 index should be `this` and when the 0 index should be the first non-this parameter in the type system.

/// `this` is the only named enum value. For all others, the underlying integer value represents the index value.

tlakollo · 2022-08-24T17:58:44Z

src/ILLink.Shared/SourceParameterIndex.cs

+	/// <example>
+	/// 0 is the first argument passed to the function in C#,
+	/// 1 is the next, and so on. These can be used to index parameters.
+	/// -1 is a flag to indicate the `this` parameter, and when SourceParamater


Nit: this comment ends abruptly in my view

Suggested change

/// -1 is a flag to indicate the `this` parameter, and when SourceParamater

/// -1 is a flag to indicate the `this` parameter, and when SourceParameter

This reverts commit 45e2e59.

This change adds ILParameterIndex to represent the IL-based indexing where 0 may refer to this, and SourceParameterIndex where 0 is the first "real" parameter in C# source code. Converting from ILParameterIndex to SourceParameterIndex returns a SourceParameterKind to differentiate between thisand other parameters and ensure that anySourceParameterIndexwill be able to indexMethodReference.Parameters`. It uses these in ParameterReferenceValue as well as GetMethodParameterValue() and adds a GetMethodThisParameterValue(), ThisParameterReferenceValue, and bubbles the changes where necessary. Most places should use SourceParameterIndex, and convert to ILParameterIndex when necessary or at the boundary of updated and old code. Commit migrated from dotnet/linker@45e2e59

jtschuster requested review from sbomer and tlakollo August 22, 2022 22:19

jtschuster requested a review from marek-safar as a code owner August 22, 2022 22:19

jtschuster requested a review from vitek-karas August 22, 2022 22:19

vitek-karas approved these changes Aug 23, 2022

View reviewed changes

sbomer approved these changes Aug 23, 2022

View reviewed changes

jtschuster marked this pull request as draft August 24, 2022 17:19

jtschuster and others added 2 commits August 24, 2022 12:22

Use enum for method parameter indexing

1ed3da8

Use different enum for this parameter and parameters

d2fa59b

jtschuster force-pushed the paramNums branch from 7e8166d to d2fa59b Compare August 24, 2022 17:38

jtschuster marked this pull request as ready for review August 24, 2022 17:51

tlakollo approved these changes Aug 24, 2022

View reviewed changes

jtschuster added 4 commits August 24, 2022 13:34

PR feedback - fix comments, remove dead code, add examples, format

0dcafb4

Add license header

8534644

Merge branch 'main' of https://github.com/dotnet/linker into paramNums

228ed7c

Fix merge issue

d0dcdc8

jtschuster merged commit 45e2e59 into dotnet:main Aug 29, 2022

jtschuster added a commit to jtschuster/linker that referenced this pull request Sep 8, 2022

Revert "Use enum for method parameter indexing (dotnet#2993)"

e8c3ea9

This reverts commit 45e2e59.

	public static SourceParameterIndex GetSourceParameter (MethodDefinition method, Instruction operation)
	public static SourceParameterIndex GetSourceParameterIndex (MethodDefinition method, Instruction operation)


		bool isByRef;

		// Thank you Cecil, Operand being a ParameterDefinition instead of an integer,

		P:Mono.Collections.Generic.Collection`1{Mono.Cecil.ParameterDefinition}.Item(System.Int32); use x
		P:Mono.Cecil.ParameterDefinitionCollection.Item(System.Int32); use x

		/// This enum and <see cref="T:Mono.Linker.ILParameterIndex"/> is used to enforce a differentiatiation between scenarios where the 0 index should be `this` and when the 0 index should be the first non-this parameter in the type system.
		/// `this` is the only named enum value. For all others, the underlying integer value represents the index value.

	/// -1 is a flag to indicate the `this` parameter, and when SourceParamater
	/// -1 is a flag to indicate the `this` parameter, and when SourceParameter

Use enum for method parameter indexing #2993

Use enum for method parameter indexing #2993

Uh oh!

Conversation

jtschuster commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbomer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtschuster commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlakollo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtschuster Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jtschuster commented Aug 22, 2022 •

edited

Loading

jtschuster commented Aug 24, 2022 •

edited

Loading

jtschuster Aug 24, 2022 •

edited

Loading