-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Conversion expression rewrite #21040
Changes from 6 commits
bc55977
f2c328d
0158878
c850505
9815e73
eb9fdb7
fcae24e
ac70ad0
2be4107
62c84ab
1094d2b
08b5bc3
28ac13f
90d781e
4d1573a
f00bd83
34a897e
c1b0ca3
aed4959
9c0a391
65f5f24
5d2ffcb
ee30c32
8a61410
901b072
d4d8834
f2d8739
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using Microsoft.CodeAnalysis.Semantics; | ||
|
||
namespace Microsoft.CodeAnalysis.CSharp | ||
{ | ||
internal abstract class BaseCSharpConversionExpression : BaseConversionExpression | ||
{ | ||
protected BaseCSharpConversionExpression(Conversion conversion, bool isExplicitInCode, bool throwsExceptionOnFailure, bool isChecked, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue) : | ||
base(conversion, isExplicitInCode, throwsExceptionOnFailure, isChecked, syntax, type, constantValue) | ||
{ | ||
ConversionInternal = conversion; | ||
} | ||
|
||
internal Conversion ConversionInternal { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cston style question: is the preferred strategy to have both the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If creating the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left it as is for now. If you'd prefer to have it created on demand, let me know and I'll change it. |
||
|
||
public override string LanguageName => LanguageNames.CSharp; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. I won't be here tomorrow, @jinujoseph can you add this to the list? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm of the opinion that we need it somewhere. Otherwise, the expected path for me to use the API is to call one of our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @CyrusNajmabadi going off @cston's moving things to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could certainly have it there (though it would add space to every conversion struct (unless you encode it as a bit)). HOwever, i think its still sensible for it to just be on IOperation like it is on ISymbol and SyntaxNode. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the |
||
} | ||
|
||
internal sealed partial class CSharpConversionExpression : BaseCSharpConversionExpression | ||
{ | ||
public CSharpConversionExpression(IOperation operand, Conversion conversion, bool isExplicit, bool throwsExceptionOnFailure, bool isChecked, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue) : | ||
base(conversion, isExplicit, throwsExceptionOnFailure, isChecked, syntax, type, constantValue) | ||
{ | ||
Operand = operand; | ||
} | ||
|
||
public override IOperation Operand { get; } | ||
} | ||
|
||
internal sealed partial class LazyCSharpConversionExpression : BaseCSharpConversionExpression | ||
{ | ||
private readonly Lazy<IOperation> _operand; | ||
public LazyCSharpConversionExpression(Lazy<IOperation> operand, Conversion conversion, bool isExplicit, bool throwsExceptionOnFailure, bool isChecked, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue) : | ||
base(conversion, isExplicit, throwsExceptionOnFailure, isChecked, syntax, type, constantValue) | ||
{ | ||
_operand = operand; | ||
} | ||
|
||
public override IOperation Operand => _operand.Value; | ||
} | ||
|
||
public static class IConversionExpressionExtensions | ||
{ | ||
/// <summary> | ||
/// Gets the underlying <see cref="Conversion"/> information from this <see cref="IConversionExpression"/>. This | ||
/// <see cref="IConversionExpression"/> must have been created from CSharp code. | ||
/// </summary> | ||
/// <param name="conversionExpression">The conversion expression to get original info from.</param> | ||
/// <returns>The underlying <see cref="Conversion"/>.</returns> | ||
/// <exception cref="InvalidCastException">If the <see cref="IConversionExpression"/> was not created from CSharp code.</exception> | ||
public static Conversion GetConversion(this IConversionExpression conversionExpression) | ||
{ | ||
if (conversionExpression is BaseCSharpConversionExpression csharpConversionExpression) | ||
{ | ||
return csharpConversionExpression.ConversionInternal; | ||
} | ||
else | ||
{ | ||
throw new ArgumentException(string.Format(CSharpResources.IConversionExpressionIsNotCSharpConversion, | ||
nameof(IConversionExpression), | ||
conversionExpression)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we relying on Are we testing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't updated the tests at all for the new conversion scheme, so this method hasn't been tested at all yet. We are currently relying on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps omit the object entirely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I think the explicit error message here will provide a better use experience. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then the exception should probably be something other than |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,4 @@ | ||
Microsoft.CodeAnalysis.CSharp.LanguageVersion.CSharp7_2 = 702 -> Microsoft.CodeAnalysis.CSharp.LanguageVersion | ||
Microsoft.CodeAnalysis.CSharp.IConversionExpressionExtensions | ||
static Microsoft.CodeAnalysis.CSharp.IConversionExpressionExtensions.GetConversion(this Microsoft.CodeAnalysis.Semantics.IConversionExpression conversionExpression) -> Microsoft.CodeAnalysis.CSharp.Conversion | ||
static Microsoft.CodeAnalysis.CSharp.Conversion.implicit operator Microsoft.CodeAnalysis.Semantics.CommonConversion(Microsoft.CodeAnalysis.CSharp.Conversion csharpConversion) -> Microsoft.CodeAnalysis.Semantics.CommonConversion |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1296,34 +1296,23 @@ public LazyConditionalChoiceExpression(Lazy<IOperation> condition, Lazy<IOperati | |
/// </summary> | ||
internal abstract partial class BaseConversionExpression : Operation, IHasOperatorMethodExpression, IConversionExpression | ||
{ | ||
protected BaseConversionExpression(ConversionKind conversionKind, bool isExplicit, bool usesOperatorMethod, IMethodSymbol operatorMethod, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue) : | ||
protected BaseConversionExpression(CommonConversion conversion, bool isExplicitInCode, bool throwsExceptionOnFailure, bool isChecked, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue) : | ||
base(OperationKind.ConversionExpression, syntax, type, constantValue) | ||
{ | ||
ConversionKind = conversionKind; | ||
IsExplicit = isExplicit; | ||
UsesOperatorMethod = usesOperatorMethod; | ||
OperatorMethod = operatorMethod; | ||
Conversion = conversion; | ||
IsExplicitInCode = isExplicitInCode; | ||
ThrowsExceptionOnFailure = throwsExceptionOnFailure; | ||
IsChecked = isChecked; | ||
} | ||
/// <summary> | ||
/// Value to be converted. | ||
/// </summary> | ||
|
||
public abstract IOperation Operand { get; } | ||
/// <summary> | ||
/// Kind of conversion. | ||
/// </summary> | ||
public ConversionKind ConversionKind { get; } | ||
/// <summary> | ||
/// True if and only if the conversion is indicated explicity by a cast operation in the source code. | ||
/// </summary> | ||
public bool IsExplicit { get; } | ||
/// <summary> | ||
/// True if and only if the operation is performed by an operator method. | ||
/// </summary> | ||
public bool UsesOperatorMethod { get; } | ||
/// <summary> | ||
/// Operation method used by the operation, null if the operation does not use an operator method. | ||
/// </summary> | ||
public IMethodSymbol OperatorMethod { get; } | ||
public CommonConversion Conversion { get; } | ||
public bool IsExplicitInCode { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this redundant with an existing boolean on IOperation? i.e. don't we have an IsImplicit boolean on IOperation we can use for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
public bool ThrowsExceptionOnFailure { get; } | ||
public bool IsChecked { get; } | ||
public abstract string LanguageName { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
public bool UsesOperatorMethod => Conversion.IsUserDefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid boxing, this should be an abstract property, implemented by the derived types. Same for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps this is a teaching moment. I would not have expected any boxing to occur here. Why would it, since everything in this chain is bool? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, IMethodSymbol will already be boxed, so I don't see any value in duplicating the code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If "Conversion" returns an IConversion, then this will box (it's an interface, they're always on the heap). If you can access something of type TConversion, then it will be a constrained call through that interface property and it will not box if the TConversion is actaully instantiated at runtime to be a struct type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but we're not talking about conversion in this comment, right? I'm seeing this on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, wait, I understand. Another argument for going with the common Conversion struct, I guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does your "Conversion" property return? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CommonConversion? |
||
public virtual IMethodSymbol OperatorMethod => Conversion.MethodSymbol; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was virtual because it needed to be redefined in the C# version, but that's not necessary any more. I'll remove it. |
||
public override IEnumerable<IOperation> Children | ||
{ | ||
get | ||
|
@@ -1341,39 +1330,6 @@ public override TResult Accept<TArgument, TResult>(OperationVisitor<TArgument, T | |
} | ||
} | ||
|
||
/// <summary> | ||
/// Represents a conversion operation. | ||
/// </summary> | ||
internal sealed partial class ConversionExpression : BaseConversionExpression, IHasOperatorMethodExpression, IConversionExpression | ||
{ | ||
public ConversionExpression(IOperation operand, ConversionKind conversionKind, bool isExplicit, bool usesOperatorMethod, IMethodSymbol operatorMethod, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue) : | ||
base(conversionKind, isExplicit, usesOperatorMethod, operatorMethod, syntax, type, constantValue) | ||
{ | ||
Operand = operand; | ||
} | ||
/// <summary> | ||
/// Value to be converted. | ||
/// </summary> | ||
public override IOperation Operand { get; } | ||
} | ||
|
||
/// <summary> | ||
/// Represents a conversion operation. | ||
/// </summary> | ||
internal sealed partial class LazyConversionExpression : BaseConversionExpression, IHasOperatorMethodExpression, IConversionExpression | ||
{ | ||
private readonly Lazy<IOperation> _lazyOperand; | ||
|
||
public LazyConversionExpression(Lazy<IOperation> operand, ConversionKind conversionKind, bool isExplicit, bool usesOperatorMethod, IMethodSymbol operatorMethod, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue) : base(conversionKind, isExplicit, usesOperatorMethod, operatorMethod, syntax, type, constantValue) | ||
{ | ||
_lazyOperand = operand ?? throw new System.ArgumentNullException(nameof(operand)); | ||
} | ||
/// <summary> | ||
/// Value to be converted. | ||
/// </summary> | ||
public override IOperation Operand => _lazyOperand.Value; | ||
} | ||
|
||
/// <remarks> | ||
/// This interface is reserved for implementation by its associated APIs. We reserve the right to | ||
/// change it in the future. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
namespace Microsoft.CodeAnalysis.Semantics | ||
{ | ||
/// <summary> | ||
/// Represents the common, language-agnostic elements of a conversion. | ||
/// </summary> | ||
/// <remarks> | ||
/// This interface is reserved for implementation by its associated APIs. We reserve the right to | ||
/// change it in the future. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll just remove it. Can't really implement a struct :). I'll remark that we reserve the right to change it in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
/// </remarks> | ||
public struct CommonConversion | ||
{ | ||
internal CommonConversion(bool exists, bool isIdentity, bool isNumeric, bool isReference, bool isUserDefined, IMethodSymbol methodSymbol) | ||
{ | ||
Exists = exists; | ||
IsIdentity = isIdentity; | ||
IsNumeric = isNumeric; | ||
IsReference = isReference; | ||
IsUserDefined = isUserDefined; | ||
MethodSymbol = methodSymbol; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
|
||
/// <summary> | ||
/// Returns true if the conversion exists, as defined by the target language. | ||
/// </summary> | ||
/// <remarks> | ||
/// The existence of a conversion does not necessarily imply that the conversion is valid. | ||
/// For example, an ambiguous user-defined conversion may exist but may not be valid. | ||
/// </remarks> | ||
public bool Exists { get; } | ||
/// <summary> | ||
/// Returns true if the conversion is an identity conversion. | ||
/// </summary> | ||
public bool IsIdentity { get; } | ||
/// <summary> | ||
/// Returns true if the conversion is a numeric conversion. | ||
/// </summary> | ||
public bool IsNumeric { get; } | ||
/// <summary> | ||
/// Returns true if the conversion is a reference conversion. | ||
/// </summary> | ||
public bool IsReference { get; } | ||
/// <summary> | ||
/// Returns true if the conversion is a user-defined conversion. | ||
/// </summary> | ||
public bool IsUserDefined { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
/// <summary> | ||
/// Returns the method used to perform the conversion for a user-defined conversion if <see cref="IsUserDefined"/> is true. | ||
/// Otherwise, returns null. | ||
/// </summary> | ||
public IMethodSymbol MethodSymbol { get; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System.Collections.Immutable; | ||
using System; | ||
|
||
namespace Microsoft.CodeAnalysis.Semantics | ||
{ | ||
|
@@ -17,14 +17,36 @@ public interface IConversionExpression : IHasOperatorMethodExpression | |
/// Value to be converted. | ||
/// </summary> | ||
IOperation Operand { get; } | ||
|
||
#pragma warning disable RS0010 // Avoid using cref tags with a prefix | ||
/// <summary> | ||
/// Kind of conversion. | ||
/// Gets the underlying common conversion information. | ||
/// </summary> | ||
ConversionKind ConversionKind { get; } | ||
/// <remarks> | ||
/// If you need conversion information that is language specific, use either | ||
/// <see cref="T:Microsoft.CodeAnalysis.CSharp.IConversionExpressionExtensions.GetConversion(IConversionExpression)"/> or | ||
/// <see cref="T:Microsoft.CodeAnalysis.VisualBasic.GetConversion(IConversionExpression)"/>. | ||
/// </remarks> | ||
#pragma warning restore RS0010 // Avoid using cref tags with a prefix | ||
CommonConversion Conversion { get; } | ||
/// <summary> | ||
/// True if and only if the conversion is indicated explicity by a cast operation in the source code. | ||
/// </summary> | ||
bool IsExplicit { get; } | ||
bool IsExplicitInCode { get; } | ||
/// <summary> | ||
/// True if the conversion will fail with a <see cref="InvalidCastException"/> at runtime if the cast fails. This is false for C#'s | ||
/// <code>as</code> operator and for VB's <code>TryCast</code> operator. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should also be false for thigns like a numeric conversion in an unchecked context. ensure we test this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. That's not going to fail with an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need a different name for this. Perhaps invert the condition and call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 for me as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will IsChecked only be true for numeric cases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @CyrusNajmabadi I'm not sure. I'll add a test case to verify that behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment appears to be stale since it says |
||
/// </summary> | ||
bool ThrowsExceptionOnFailure { get; } | ||
/// <summary> | ||
/// True if the conversion can fail at runtime with an overflow exception. This corresponds to C# checked and unchecked blocks. | ||
/// </summary> | ||
bool IsChecked { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'ts not clear ot me how this property and the one above are related. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're different failure modes, and I'm not of the opinion we should try to unify them. |
||
/// <summary> | ||
/// The language that defined this conversion. Possible values are <see cref="LanguageNames.CSharp"/> and | ||
/// <see cref="LanguageNames.VisualBasic"/>. | ||
/// </summary> | ||
string LanguageName { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can all of these properties move to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cston I'm not sure. As I mentioned to @CyrusNajmabadi below, I'm ok with moving |
||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the implicit operator necessary? Consider making this an explicit method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed