-
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
Diagnostic Improvements for error #0281 #27052
Changes from 17 commits
37ed85e
6eb8ea7
de25429
ee189b0
ab9faaf
af6f5ac
0b3c9f1
080de89
3191a12
44e6815
e24f1a6
cf7eb19
200acb1
816472a
6f57496
56bb172
7eba065
72b880a
28f0a80
15176b2
6a36501
e9baab9
b095767
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,6 @@ | |
using System.Collections.Immutable; | ||
using System.Diagnostics; | ||
using System.Linq; | ||
using Microsoft.CodeAnalysis.Collections; | ||
using Microsoft.CodeAnalysis.CSharp.Symbols; | ||
using Microsoft.CodeAnalysis.PooledObjects; | ||
using Roslyn.Utilities; | ||
|
@@ -1154,17 +1153,24 @@ internal SingleLookupResult CheckViability(Symbol symbol, int arity, LookupOptio | |
ref useSiteDiagnostics, | ||
basesBeingResolved)) | ||
{ | ||
if (inaccessibleViaQualifier) | ||
{ | ||
diagInfo = diagnose ? new CSDiagnosticInfo(ErrorCode.ERR_BadProtectedAccess, unwrappedSymbol, accessThroughType, this.ContainingType) : null; | ||
} | ||
else | ||
{ | ||
var unwrappedSymbols = ImmutableArray.Create<Symbol>(unwrappedSymbol); | ||
diagInfo = diagnose ? new CSDiagnosticInfo(ErrorCode.ERR_BadAccess, new[] { unwrappedSymbol }, unwrappedSymbols, additionalLocations: ImmutableArray<Location>.Empty) : null; | ||
} | ||
|
||
return LookupResult.Inaccessible(symbol, diagInfo); | ||
if (!diagnose) | ||
{ | ||
diagInfo = null; | ||
} | ||
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 be combined with
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. That would capture 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. My mistake, there is no |
||
else if (inaccessibleViaQualifier) | ||
{ | ||
diagInfo = new CSDiagnosticInfo(ErrorCode.ERR_BadProtectedAccess, unwrappedSymbol, accessThroughType, this.ContainingType); | ||
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. Indent is off here. currently six spaces, should be four. |
||
} | ||
else if (IsBadIvtSpecification()) | ||
{ | ||
diagInfo = new CSDiagnosticInfo(ErrorCode.ERR_FriendRefNotEqualToThis, unwrappedSymbol.ContainingAssembly.Identity.ToString(), AssemblyIdentity.PublicKeyToString(this.Compilation.Assembly.PublicKey)); | ||
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. Indent is off here. Currently seven spaces, should be four., |
||
} | ||
else | ||
{ | ||
diagInfo = new CSDiagnosticInfo(ErrorCode.ERR_BadAccess, new[] { unwrappedSymbol }, ImmutableArray.Create<Symbol>(unwrappedSymbol), additionalLocations: ImmutableArray<Location>.Empty); | ||
} | ||
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. Consider checking
|
||
|
||
return LookupResult.Inaccessible(symbol, diagInfo); | ||
} | ||
else if (!InCref && unwrappedSymbol.MustCallMethodsDirectly()) | ||
{ | ||
|
@@ -1195,8 +1201,29 @@ internal SingleLookupResult CheckViability(Symbol symbol, int arity, LookupOptio | |
{ | ||
return LookupResult.Good(symbol); | ||
} | ||
} | ||
|
||
bool IsBadIvtSpecification() | ||
{ | ||
// Ensures that during binding we don't ask for public key which results in attribute binding and stack overflow. | ||
// If looking up attributes, don't ask for public key. | ||
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 these comment lines are out of order (it reads better if the lower one is at the top). |
||
if ((unwrappedSymbol.DeclaredAccessibility == Accessibility.Internal || | ||
unwrappedSymbol.DeclaredAccessibility == Accessibility.ProtectedAndInternal || | ||
unwrappedSymbol.DeclaredAccessibility == Accessibility.ProtectedOrInternal) | ||
&& !options.IsAttributeTypeLookup()) | ||
{ | ||
foreach (ImmutableArray<byte> key in unwrappedSymbol.ContainingAssembly.GetInternalsVisibleToPublicKeys(this.Compilation.AssemblyName)) | ||
{ | ||
if (key.SequenceEqual(this.Compilation.Assembly.Identity.PublicKey)) | ||
{ | ||
return false; | ||
} | ||
} | ||
return true; | ||
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. This returns true even if there are no IVT attributes. The check should be that there is at least one attribute, but all of the public keys are wrong. 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. line 1221 is nested within the greater Accessibility check statement, and is only called if all of the SequenceEquals have returned false / the public keys are wrong. There is a return false at line 1223, outside of the accessibility check. Are you referring to different attributes? 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. DeclaredAccessibility just means what it was declared as, e.g. 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, then how does one test for attributes? 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 you can just check if the thing you're going to foreach over is empty before trying and return false if so. 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. Pushed |
||
} | ||
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 |
||
return false; | ||
} | ||
} | ||
|
||
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. Accidental whitespace change. |
||
private CSDiagnosticInfo MakeCallMethodsDirectlyDiagnostic(Symbol symbol) | ||
{ | ||
Debug.Assert(symbol.MustCallMethodsDirectly()); | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Indent still seems off here. Currently three spaces, should be four.