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.

Sign up for GitHub

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

Jump to bottom

Enable more Roslynator rules #21192

Merged
merged 6 commits into from
Nov 10, 2023
Merged

Enable more Roslynator rules #21192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
844e7d0
Fix RCS1061
RoosterDragon Mar 18, 2023
4fe8af7
Fix RCS1089
RoosterDragon Mar 18, 2023
7ed3531
Fix RCS1118
RoosterDragon Mar 18, 2023
85707e5
Fix RCS1170
RoosterDragon Mar 18, 2023
766f83e
Fix RCS1205
RoosterDragon Mar 18, 2023
3dff038
Fix RCS1236
RoosterDragon Aug 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions .editorconfig
View file
Open in desktop
Expand Up @@ -980,6 +980,9 @@ dotnet_diagnostic.RCS1058.severity = warning
# Avoid locking on publicly accessible instance.
dotnet_diagnostic.RCS1059.severity = warning

# Merge 'if' with nested 'if'.
dotnet_diagnostic.RCS1061.severity = warning

# Remove empty 'finally' clause.
dotnet_diagnostic.RCS1066.severity = warning

Expand All @@ -1001,6 +1004,9 @@ dotnet_diagnostic.RCS1080.severity = warning
# Use coalesce expression instead of conditional expression.
dotnet_diagnostic.RCS1084.severity = warning

# Use --/++ operator instead of assignment.
dotnet_diagnostic.RCS1089.severity = warning

# Remove empty region.
dotnet_diagnostic.RCS1091.severity = warning

Expand All @@ -1022,6 +1028,9 @@ dotnet_diagnostic.RCS1112.severity = warning
# Use 'string.IsNullOrEmpty' method.
dotnet_diagnostic.RCS1113.severity = warning

# Mark local variable as const.
dotnet_diagnostic.RCS1118.severity = warning

# Bitwise operation on enum without Flags attribute.
dotnet_diagnostic.RCS1130.severity = warning

Expand Down Expand Up @@ -1052,6 +1061,9 @@ dotnet_diagnostic.RCS1159.severity = warning
# Unused type parameter.
dotnet_diagnostic.RCS1164.severity = warning

# Use read-only auto-implemented property.
dotnet_diagnostic.RCS1170.severity = warning
Copy link
Member

Choose a reason for hiding this comment

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

it looks like this rule was implemented incorrectly. Private setters were made public.

I'm not sure how this rule exactly works as it affected so little code, but this could make it harder to work with magic. In trait infos we use auto-implemented properties for non-yaml fields

Copy link
Member

Choose a reason for hiding this comment

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

Ah mb I misunderstood, but it did indeed remove the privateness from RadarWidget

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 rule wants to change public string SoundUp { get; private set; } to public string SoundUp { get; }. However, this breaks our use of reflection in FieldLoader/ObjectCreator as it sets SoundUp from YAML.

I manually changed it to a field, like other things that need to be loaded from YAML, as it was the most elegant workaround I could see. This seemed like a worthwhile tradeoff so we can still enable an otherwise useful rule.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at git blame yeah this is supposed to be used by reflection. I thought it was purposefully added to hide it from the reflection. 85b9cf0 7eb64ea

Copy link
Member

Choose a reason for hiding this comment

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

actually should have made these readonly to match other yaml defined values


# Use 'is' operator instead of 'as' operator.
dotnet_diagnostic.RCS1172.severity = warning

Expand Down Expand Up @@ -1085,6 +1097,9 @@ dotnet_diagnostic.RCS1199.severity = warning
# Use EventArgs.Empty.
dotnet_diagnostic.RCS1204.severity = warning

# Order named arguments according to the order of parameters.
dotnet_diagnostic.RCS1205.severity = warning

# Order type parameter constraints.
dotnet_diagnostic.RCS1209.severity = warning

Expand Down Expand Up @@ -1121,5 +1136,9 @@ dotnet_diagnostic.RCS1233.severity = warning
# Optimize method call.
dotnet_diagnostic.RCS1235.severity = warning

# Use exception filter.
dotnet_diagnostic.RCS1236.severity = warning

# Use 'for' statement instead of 'while' statement.
dotnet_diagnostic.RCS1239.severity = warning

2 changes: 1 addition & 1 deletion OpenRA.Game/Actor.cs
View file
Open in desktop
Expand Up @@ -71,7 +71,7 @@ public Activity CurrentActivity
public IEffectiveOwner EffectiveOwner { get; }
public IOccupySpace OccupiesSpace { get; }
public ITargetable[] Targetables { get; }
public IEnumerable<ITargetablePositions> EnabledTargetablePositions { get; private set; }
public IEnumerable<ITargetablePositions> EnabledTargetablePositions { get; }

public bool IsIdle => CurrentActivity == null;
public bool IsDead => Disposed || (health != null && health.IsDead);
Expand Down
8 changes: 4 additions & 4 deletions OpenRA.Game/Exts.cs
View file
Open in desktop
Expand Up @@ -302,9 +302,9 @@ public static uint ISqrt(uint number, ISqrtRoundMode round = ISqrtRoundMode.Floo

// Adjust for other rounding modes
if (round == ISqrtRoundMode.Nearest && remainder > root)
root += 1;
root++;
else if (round == ISqrtRoundMode.Ceiling && root * root < number)
root += 1;
root++;

return root;
}
Expand Down Expand Up @@ -343,9 +343,9 @@ public static ulong ISqrt(ulong number, ISqrtRoundMode round = ISqrtRoundMode.Fl

// Adjust for other rounding modes
if (round == ISqrtRoundMode.Nearest && remainder > root)
root += 1;
root++;
else if (round == ISqrtRoundMode.Ceiling && root * root < number)
root += 1;
root++;

return root;
}
Expand Down
38 changes: 17 additions & 21 deletions OpenRA.Game/FieldLoader.cs
View file
Open in desktop
Expand Up @@ -195,11 +195,11 @@ static object ParseWVec(string fieldName, Type fieldType, string value, MemberIn
if (value != null)
{
var parts = value.Split(SplitComma);
if (parts.Length == 3)
{
if (WDist.TryParse(parts[0], out var rx) && WDist.TryParse(parts[1], out var ry) && WDist.TryParse(parts[2], out var rz))
return new WVec(rx, ry, rz);
}
if (parts.Length == 3
&& WDist.TryParse(parts[0], out var rx)
&& WDist.TryParse(parts[1], out var ry)
&& WDist.TryParse(parts[2], out var rz))
return new WVec(rx, ry, rz);
}

return InvalidValueAction(value, fieldType, fieldName);
Expand All @@ -219,8 +219,8 @@ static object ParseWVecArray(string fieldName, Type fieldType, string value, Mem
for (var i = 0; i < vecs.Length; ++i)
{
if (WDist.TryParse(parts[3 * i], out var rx)
&& WDist.TryParse(parts[3 * i + 1], out var ry)
&& WDist.TryParse(parts[3 * i + 2], out var rz))
&& WDist.TryParse(parts[3 * i + 1], out var ry)
&& WDist.TryParse(parts[3 * i + 2], out var rz))
vecs[i] = new WVec(rx, ry, rz);
}

Expand All @@ -235,13 +235,11 @@ static object ParseWPos(string fieldName, Type fieldType, string value, MemberIn
if (value != null)
{
var parts = value.Split(SplitComma);
if (parts.Length == 3)
{
if (WDist.TryParse(parts[0], out var rx)
&& WDist.TryParse(parts[1], out var ry)
&& WDist.TryParse(parts[2], out var rz))
return new WPos(rx, ry, rz);
}
if (parts.Length == 3
&& WDist.TryParse(parts[0], out var rx)
&& WDist.TryParse(parts[1], out var ry)
&& WDist.TryParse(parts[2], out var rz))
return new WPos(rx, ry, rz);
}

return InvalidValueAction(value, fieldType, fieldName);
Expand All @@ -259,13 +257,11 @@ static object ParseWRot(string fieldName, Type fieldType, string value, MemberIn
if (value != null)
{
var parts = value.Split(SplitComma);
if (parts.Length == 3)
{
if (Exts.TryParseInt32Invariant(parts[0], out var rr)
&& Exts.TryParseInt32Invariant(parts[1], out var rp)
&& Exts.TryParseInt32Invariant(parts[2], out var ry))
return new WRot(new WAngle(rr), new WAngle(rp), new WAngle(ry));
}
if (parts.Length == 3
&& Exts.TryParseInt32Invariant(parts[0], out var rr)
&& Exts.TryParseInt32Invariant(parts[1], out var rp)
&& Exts.TryParseInt32Invariant(parts[2], out var ry))
return new WRot(new WAngle(rr), new WAngle(rp), new WAngle(ry));
}

return InvalidValueAction(value, fieldType, fieldName);
Expand Down
23 changes: 9 additions & 14 deletions OpenRA.Game/FileSystem/FileSystem.cs
View file
Open in desktop
Expand Up @@ -109,10 +109,8 @@ public void Mount(string name, string explicitName = null)

Mount(package, explicitName);
}
catch
catch when (optional)
{
if (!optional)
throw;
}
}

Expand Down Expand Up @@ -226,14 +224,11 @@ public bool TryGetPackageContaining(string path, out IReadOnlyPackage package, o
public bool TryOpen(string filename, out Stream s)
{
var explicitSplit = filename.IndexOf('|');
if (explicitSplit > 0)
if (explicitSplit > 0 && explicitMounts.TryGetValue(filename[..explicitSplit], out var explicitPackage))
{
if (explicitMounts.TryGetValue(filename[..explicitSplit], out var explicitPackage))
{
s = explicitPackage.GetStream(filename[(explicitSplit + 1)..]);
if (s != null)
return true;
}
s = explicitPackage.GetStream(filename[(explicitSplit + 1)..]);
if (s != null)
return true;
}

s = GetFromCache(filename);
Expand Down Expand Up @@ -262,10 +257,10 @@ public bool TryOpen(string filename, out Stream s)
public bool Exists(string filename)
{
var explicitSplit = filename.IndexOf('|');
if (explicitSplit > 0)
if (explicitMounts.TryGetValue(filename[..explicitSplit], out var explicitPackage))
if (explicitPackage.Contains(filename[(explicitSplit + 1)..]))
return true;
if (explicitSplit > 0 &&
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if we could be consistent about whether the && goes at the start or end of the line in these cases. Is there a rule for that?

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'm not aware of one offhand, but that's not to say there isn't one somewhere.

I've generally wrapped after as I think that's the more common convention in the codebase, but even in this chageset I wrap before in FieldLoader as that's the more common convention in that file.

Copy link
Member

Choose a reason for hiding this comment

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

I usually find the && or || nicer and more readable when placed at the start. Though it does depend on the length of the first line

explicitMounts.TryGetValue(filename[..explicitSplit], out var explicitPackage) &&
explicitPackage.Contains(filename[(explicitSplit + 1)..]))
return true;

return fileIndex.ContainsKey(filename);
}
Expand Down
10 changes: 5 additions & 5 deletions OpenRA.Game/Graphics/Sprite.cs
View file
Open in desktop
Expand Up @@ -42,11 +42,11 @@ public Sprite(Sheet sheet, Rectangle bounds, float zRamp, in float3 offset, Text
// in rendering a line of texels that sample outside the sprite rectangle.
// Insetting the texture coordinates by a small fraction of a pixel avoids this
// with negligible impact on the 1:1 rendering case.
var inset = 1 / 128f;
Left = (Math.Min(bounds.Left, bounds.Right) + inset) / sheet.Size.Width;
Top = (Math.Min(bounds.Top, bounds.Bottom) + inset) / sheet.Size.Height;
Right = (Math.Max(bounds.Left, bounds.Right) - inset) / sheet.Size.Width;
Bottom = (Math.Max(bounds.Top, bounds.Bottom) - inset) / sheet.Size.Height;
const float Inset = 1 / 128f;
Left = (Math.Min(bounds.Left, bounds.Right) + Inset) / sheet.Size.Width;
Top = (Math.Min(bounds.Top, bounds.Bottom) + Inset) / sheet.Size.Height;
Right = (Math.Max(bounds.Left, bounds.Right) - Inset) / sheet.Size.Width;
Bottom = (Math.Max(bounds.Top, bounds.Bottom) - Inset) / sheet.Size.Height;
}
}

Expand Down
4 changes: 2 additions & 2 deletions OpenRA.Game/Map/CellRegion.cs
View file
Open in desktop
Expand Up @@ -135,12 +135,12 @@ public CellRegionEnumerator(CellRegion region)

public bool MoveNext()
{
u += 1;
u++;

// Check for column overflow
if (u > r.mapBottomRight.U)
{
v += 1;
v++;
u = r.mapTopLeft.U;

// Check for row overflow
Expand Down
16 changes: 8 additions & 8 deletions OpenRA.Game/Map/Map.cs
View file
Open in desktop
Expand Up @@ -611,7 +611,7 @@ PPos[] ProjectCellInner(MPos uv)

// Odd-height ramps get bumped up a level to the next even height layer
if ((height & 1) == 1 && Ramp[uv] != 0)
height += 1;
height++;

var candidates = new List<PPos>();

Expand Down Expand Up @@ -685,16 +685,16 @@ public byte[] SaveBinaryData()
writer.Write((ushort)MapSize.Y);

// Data offsets
var tilesOffset = 17;
const int TilesOffset = 17;
var heightsOffset = Grid.MaximumTerrainHeight > 0 ? 3 * MapSize.X * MapSize.Y + 17 : 0;
var resourcesOffset = (Grid.MaximumTerrainHeight > 0 ? 4 : 3) * MapSize.X * MapSize.Y + 17;

writer.Write((uint)tilesOffset);
writer.Write((uint)TilesOffset);
writer.Write((uint)heightsOffset);
writer.Write((uint)resourcesOffset);

// Tile data
if (tilesOffset != 0)
if (TilesOffset != 0)
{
for (var i = 0; i < MapSize.X; i++)
{
Expand Down Expand Up @@ -805,7 +805,7 @@ public byte[] SavePreview()
bitmapWidth = 2 * bitmapWidth - 1;

var stride = bitmapWidth * 4;
var pxStride = 4;
const int PxStride = 4;
var minimapData = new byte[stride * height];
(Color Left, Color Right) terrainColor = default;

Expand All @@ -827,10 +827,10 @@ public byte[] SavePreview()
{
// Odd rows are shifted right by 1px
var dx = uv.V & 1;
var xOffset = pxStride * (2 * x + dx);
var xOffset = PxStride * (2 * x + dx);
if (x + dx > 0)
{
var z = y * stride + xOffset - pxStride;
var z = y * stride + xOffset - PxStride;
var c = actorColor.A == 0 ? terrainColor.Left : actorColor;
minimapData[z++] = c.R;
minimapData[z++] = c.G;
Expand All @@ -850,7 +850,7 @@ public byte[] SavePreview()
}
else
{
var z = y * stride + pxStride * x;
var z = y * stride + PxStride * x;
var c = actorColor.A == 0 ? terrainColor.Left : actorColor;
minimapData[z++] = c.R;
minimapData[z++] = c.G;
Expand Down
10 changes: 5 additions & 5 deletions OpenRA.Game/Map/MapCache.cs
View file
Open in desktop
Expand Up @@ -282,11 +282,11 @@ void LoadAsyncInternal()
Log.Write("debug", "MapCache.LoadAsyncInternal started");

// Milliseconds to wait on one loop when nothing to do
var emptyDelay = 50;
const int EmptyDelay = 50;

// Keep the thread alive for at least 5 seconds after the last minimap generation
var maxKeepAlive = 5000 / emptyDelay;
var keepAlive = maxKeepAlive;
const int MaxKeepAlive = 5000 / EmptyDelay;
var keepAlive = MaxKeepAlive;

while (true)
{
Expand All @@ -306,11 +306,11 @@ void LoadAsyncInternal()

if (todo.Count == 0)
{
Thread.Sleep(emptyDelay);
Thread.Sleep(EmptyDelay);
continue;
}
else
keepAlive = maxKeepAlive;
keepAlive = MaxKeepAlive;

// Render the minimap into the shared sheet
foreach (var p in todo)
Expand Down
2 changes: 1 addition & 1 deletion OpenRA.Game/Map/MapCoordsRegion.cs
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@ public bool MoveNext()
// Check for column overflow
if (u > r.BottomRight.U)
{
v += 1;
v++;
u = r.TopLeft.U;

// Check for row overflow
Expand Down
4 changes: 2 additions & 2 deletions OpenRA.Game/Map/ProjectedCellRegion.cs
View file
Open in desktop
Expand Up @@ -93,12 +93,12 @@ public ProjectedCellRegionEnumerator(ProjectedCellRegion region)

public bool MoveNext()
{
u += 1;
u++;

// Check for column overflow
if (u > r.BottomRight.U)
{
v += 1;
v++;
u = r.TopLeft.U;

// Check for row overflow
Expand Down
5 changes: 2 additions & 3 deletions OpenRA.Game/Network/GameServer.cs
View file
Open in desktop
Expand Up @@ -169,9 +169,8 @@ public GameServer(MiniYaml yaml)
}

// Games advertised using the old API calculated the play time locally
if (State == 2 && PlayTime < 0)
if (DateTime.TryParse(Started, out var startTime))
PlayTime = (int)(DateTime.UtcNow - startTime).TotalSeconds;
if (State == 2 && PlayTime < 0 && DateTime.TryParse(Started, out var startTime))
PlayTime = (int)(DateTime.UtcNow - startTime).TotalSeconds;

var externalKey = ExternalMod.MakeKey(Mod, Version);
if (Game.ExternalMods.TryGetValue(externalKey, out var external) && external.Version == Version)
Expand Down
4 changes: 2 additions & 2 deletions OpenRA.Game/ObjectCreator.cs
View file
Open in desktop
Expand Up @@ -133,8 +133,8 @@ public Type FindType(string className)

public ConstructorInfo GetCtor(Type type)
{
var flags = BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance;
var ctors = type.GetConstructors(flags).Where(x => x.HasAttribute<UseCtorAttribute>()).ToList();
const BindingFlags Flags = BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance;
var ctors = type.GetConstructors(Flags).Where(x => x.HasAttribute<UseCtorAttribute>()).ToList();
if (ctors.Count > 1)
throw new InvalidOperationException("ObjectCreator: UseCtor on multiple constructors; invalid.");
return ctors.FirstOrDefault();
Expand Down
2 changes: 1 addition & 1 deletion OpenRA.Game/Primitives/Color.cs
View file
Open in desktop
Expand Up @@ -154,7 +154,7 @@ public static (float H, float S, float V) RgbToHsv(float r, float g, float b)

// Wrap negative values into [0-1)
if (h < 0)
h += 1;
h++;

var s = delta / rgbMax;
return (h, s, v);
Expand Down
4 changes: 2 additions & 2 deletions OpenRA.Game/Scripting/ScriptMemberWrapper.cs
View file
Open in desktop
Expand Up @@ -125,8 +125,8 @@ public void Set(LuaValue value)
public static IEnumerable<MemberInfo> WrappableMembers(Type t)
{
// Only expose defined public non-static methods that were explicitly declared by the author
var flags = BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly;
foreach (var mi in t.GetMembers(flags))
const BindingFlags Flags = BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly;
foreach (var mi in t.GetMembers(Flags))
{
// Properties are always wrappable
if (mi is PropertyInfo)
Expand Down