Geekly Articles each Day
📜 ⬆️ ⬇️

Unicorn in space: check the source code of 'Space Engineers'



As you already understood from the title, this article will talk about suspicious places found in the source code of the 'Space Engineers'. But the format of the article is somewhat different from the rest. In addition to information about the project, a review of some of the suspicious places and errors found, as well as ways to correct them, I included in the text a small section on the correct scenario of using a static analyzer. I strongly recommend that you familiarize yourself with it, since many developers do not know or simply do not think about how to properly use the tools of this class. As a result, static analysis tools are used an order of magnitude less efficiently than they could.

A little about the game




Space Engineers is a sandbox about building and maintaining space objects. Players can build space ships and stations of various sizes and purposes (civilian and military), manage them and extract resources. The game uses a realistic physics engine: any object can be assembled, disassembled, damaged and completely destroyed. The game is based on the graphics engine VRage 2.0 of its own, developed by the creator company Keen Software House .
')
The source code of the game is available in the repository on GitHub .

The source code is written in C # and verified using the PVS-Studio static code analyzer. You can check the project that interests you by downloading and trying the analyzer on your own or someone else's code.

The general list of found errors and projects in which they were identified can be found on the corresponding link .

Wrong and suspicious places


Below are some of the warnings that the analyzer issued, as well as code fragments that it considered suspicious. I repeat that these are not all warnings. More information about the total number of errors and why not all of them were considered, is written in the appropriate section.

But you already want to look at the code, right? Well, let's get started.

Typos and inattentive 'copy-paste'


void DeserializeV0(XmlReader reader) { .... if (property.Name == "Rotation" || property.Name == "AxisScale" || property.Name == "AxisScale") continue; .... }

PVS-Studio warning : V3001 There are identical sub-expressions 'property.Name == "AxisScale"' to the left and to the right of the '||' operator. Sandbox.Graphics MyParticleEmitter.cs 352

A typical mistake that occurs in C ++ code, C # code, and, probably, in many other programming languages. The reason for such errors, as a rule, is a simple inattention. Compared the property 'property.Name' with string literals and twice compared with 'AxisScale'. Most likely, a comparison with another literal was implied (in neighboring methods this property is compared with the literal 'LimitAngle', so I think it was meant here).

Not less typical errors of the same bodies of the 'if' operator in the 'then' and 'else'-blocks were encountered. Such errors arise due to inattention (including due to inattention copy-paste ). Consider a few similar examples:

 private void StartRespawn() { m_lastCountdownTime = MySandboxGame.TotalGamePlayTimeInMilliseconds; if (m_removeAfterDeath) m_deathCountdownMs = AgentDefinition.RemoveTimeMs; else m_deathCountdownMs = AgentDefinition.RemoveTimeMs; }

PVS-Studio warning : V3004 The 'then' statement is equivalent to the 'else' statement. Sandbox.Game MyAgentBot.cs 260

Regardless of the value of the 'm_removeAfterDeath' variable, the other variable - 'm_deathCountdownMs' - will be assigned the same value. In this case, it is difficult to say what needs to be changed here. But the fact that the code contains an error is obvious.

The following similar fragment:

 private static bool IsTriangleDangerous(int triIndex) { if (MyPerGameSettings.NavmeshPresumesDownwardGravity) { return triIndex == -1; } else { return triIndex == -1; } }

PVS-Studio warning : V3004 The 'then' statement is equivalent to the 'else' statement. Sandbox.Game MyNavigationTriangle.cs 189

The situation is similar to the previous one, the use of the 'if' operator in this code does not make sense. And again it's hard to say how to fix this code. Perhaps, depending on the condition, it is necessary to use either the '==' operator, or '! ='. But this is only an assumption.

And one more similar situation:

 public void UpdateLight() { .... if (((MyCubeGrid)Parent).GridSizeEnum == MyCubeSize.Large) Light.GlareIntensity = 0.5f + length * 2; else Light.GlareIntensity = 0.5f + length * 2; .... }

PVS-Studio warning : V3004 The 'then' statement is equivalent to the 'else' statement. Sandbox.Game MyThrust.cs 149

Depending on the condition, it is necessary to change the intensity of the glare. But due to copy-paste it will always be the same. What value needs to be set in this or that case again cannot be said, it is up to the authors of the code.

Loss of return values


There is code in projects that does not use the return value of the method. This may be due, for example, to the fact that the programmer has forgotten that the 'Replace' method of the 'String' class returns the modified string, and the original remains intact, since the objects of the 'String' class are immutable. This project also encountered 2 errors associated with the loss of the value returned by the method:

 public void Init(string cueName) { .... if (m_arcade.Hash == MyStringHash.NullOrEmpty && m_realistic.Hash == MyStringHash.NullOrEmpty) MySandboxGame.Log.WriteLine(string.Format( "Could not find any sound for '{0}'", cueName)); else { if (m_arcade.IsNull) string.Format( "Could not find arcade sound for '{0}'", cueName); if (m_realistic.IsNull) string.Format( "Could not find realistic sound for '{0}'", cueName); } }

PVS-Studio warnings :
The static method 'Format' of the class 'String' makes up the result string based on the format string and the arguments that form it, and then returns the result string. Therefore, calling this method without using the return value does not make sense.

From this code, it is clear that in case some elements could not be found, it is necessary to write error information to the log. And the last two calls to the 'string.Format' method should be arguments to the 'MySandboxGame.Log.WriteLine' method.

The correct code might look like this:

 if (m_arcade.IsNull) MySandboxGame.Log.WriteLine(string.Format( "Could not find arcade sound for '{0}'", cueName)); if (m_realistic.IsNull) MySandboxGame.Log.WriteLine(string.Format( "Could not find realistic sound for '{0}'", cueName));

Incorrect verification after using the 'as' operator


In my other articles about checking C # projects (' Checking the source code of C # /. NET components from Sony ', ' Looking for bugs in MonoDevelop '), I mentioned that among some bugs that C # programmers make, certain patterns. And with each new proven project, I am more and more convinced of this. One of them is to cast an object with the help of the 'as' operator to a compatible type, and after that - check for the 'null' not of the received object, but of the original one, which causes the likelihood of exceptions of the type 'NullReferenceException'. "Space engineers" did not stand aside.

Let's look at some examples with similar errors:

 protected override void Init(MyObjectBuilder_DefinitionBase builder) { base.Init(builder); var ob = builder as MyObjectBuilder_WeaponBlockDefinition; Debug.Assert(builder != null); WeaponDefinitionId = new MyDefinitionId(ob.WeaponDefinitionId.Type, ob.WeaponDefinitionId.Subtype); ResourceSinkGroup = MyStringHash.GetOrCompute(ob.ResourceSinkGroup); InventoryMaxVolume = ob.InventoryMaxVolume; }

PVS-Studio warning : V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'builder', 'ob'. Sandbox.Game MyWeaponBlockDefinition.cs 21

This code will work correctly if the builder is equal to null. Then the Assert will work and everyone will be happy (relatively, of course). If the 'builder' is of the type 'MyObjectBuilder_WeaponBlockDefinition', everything is fine again. But if 'builder' has a value not 'null', but as a result of casting, the object 'ob' will have the value 'null' - the check 'Debug.Assert (builder! = Null)' will pass successfully, and below, when using the object 'ob' will throw an exception of type 'NullReferenceException'.

I have “chewed up” some of the cases where the code will work correctly, and when it will not, in order not to repeat these explanations later. But, I think, it is obvious that such code contains an error.

Similar error:

 private void contextMenu_ItemClicked(MyGuiControlContextMenu sender, MyGuiControlContextMenu.EventArgs args) { .... var actionsItem = item as MyToolbarItemActions; if (item != null) { if (idx < 0 || idx >= actionsItem .PossibleActions(ShownToolbar.ToolbarType) .Count) RemoveToolbarItem(slot); .... } .... }

PVS-Studio warning : V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'item', 'actionsItem'. Sandbox.Game MyGuiControlToolbar.cs 511

If the item object could not be cast to the 'MyToolbarItemActions' and 'actionsItem' types, checking the 'item! = Null' will not help, since the wrong object is checked, and an exception of the 'NullReferenceException' type may occur below.
Then it would be correct to correct the check for this:

 if (actionsItem != null)

Several warnings with similar cases:

Suspicious comparisons


In the PVS-Studio 6.01 version, in addition to the newly added ones, existing diagnostics have been improved, and some of them have been significantly improved. An example of one of these diagnostics is V3022 , which determines that the condition will always be true or false.
I propose to consider several fragments of a similar code, marked by the analyzer:

 private long SpawnInventoryContainer(MyDefinitionId bagDefinition) { ... } public override void OnCharacterDead() { .... var bagEntityId = SpawnInventoryContainer( Character.Definition.InventorySpawnContainerId.Value); if (bagEntityId != null) .... }

PVS-Studio warning : V3022 Expression 'bagEntityId! = Null' is always true. Sandbox.Game MyCharacterInventorySpawnComponent.cs 60

Since the 'SpawnInventoryContainer' method returns an object of type 'long', the variable 'bagEntityId' will have the same type. Primitive types like 'long' can be compared with 'null' (long_var == null), but the result of this comparison will always be 'false'. As a consequence, the body of the 'if' operator will always be executed. Most likely, the expected nullable-type - 'long?'.

This is not the only such case, there were more places where primitive significant types were compared to 'null'. Relevant warning analyzer:


There are other interesting code snippets:

 private new bool TestPlacement() { .... for (int i = 0; i < PreviewGrids.Count; ++i) { .... if (retval && i == 0) { .... var settings = i == 0 ? m_settings.GetGridPlacementSettings(grid, false) : MyPerGameSettings.BuildingSettings.SmallStaticGrid; .... } .... } }

PVS-Studio warning : V3022 Expression 'i == 0' is always true. Sandbox.Game MyGridClipboardAdvanced.cs 790

Also a very interesting case. There is a ternary operator in the code, but there is no sense from it. In the condition of the 'if' operator, it is checked that 'i == 0', and then, when the 'settings' object is initialized, this condition is checked again. It would be logical, but between these parts of the code the 'i' does not change at all, as a result - there is no point in checking, the 'settings' will be constantly initialized with the same value.

There were more warnings in the same cycle:


The code has met several dozen such warnings. It makes no sense to consider all of them here, if you wish, you can download the project source code and the analyzer and check the project yourself (there are links to the code and the analyzer at the beginning of the article). Fortunately, the project is assembled and analyzed simply and quickly. Kill several birds with one stone - try the analyzer, see the benefits of using tools of this kind, and at the same time familiarize yourself with the source code of the project.

And again about dereferencing null links


Despite the fact that C # in terms of referring to null links is much safer than dereferencing null pointers in C ++ (leading to UB ), the unexpected generation of exceptions like 'NullReferenceException' is still extremely unpleasant, especially if these exceptions occur to users rather than show yourself at the design stage. Therefore, if there is a possibility of dereferencing the zero reference, you need to be extremely attentive:

 new MyEntity Entity { get; } private static bool EnergyCritWarningMethod(out MyGuiSounds cue, out MyStringId text) { .... if (MySession.ControlledEntity.Entity is MyCharacter || MySession.ControlledEntity == null) .... }

PVS-Studio warning : V3027 The variable 'MySession.ControlledEntity' has been verified. Sandbox.Game MyHudWarning.cs 415

There is a need to perform some actions if 'MySession.ControlledEntity == null' or 'MySession.ControlledEntity.Entity' is a type compatible with 'MyCharacter'. But since the verification of these conditions is in the wrong order, an exception may occur. It will be generated if 'MySession.ControlledEntity == null', since 'Entity' is an instance property. The solution is to change the order of the subexpressions:

 if (MySession.ControlledEntity == null || MySession.ControlledEntity.Entity is MyCharacter)


Strange cycles


There are also errors with cycles in the code, for example, when the body of the cycle is never executed, it will always be executed strictly once, or it will be executed indefinitely. There are many reasons, and they are all different. I propose to look at one of these cycles:

 internal static void AddDivisionForCullingStructure(List<MyRenderObject> roList, int objectCountLimit, List<BoundingBoxD> resultDivision) { .... for (int axis = bestAxis; axis <= bestAxis; axis++) .... }

PVS-Studio warning : V3028 Consider inspecting the 'for' operator. Initial and final values ​​of the iterator are the same. VRage.Render MyRender-Management.cs 1034

The loop counter ('axis') is initialized with the value of 'bestAxis', but the exit condition is the same or a smaller value, due to which no iteration of the loop will be performed. The implication was that the counter should initialize 0, then the loop would look like this:

 for (int axis = 0; axis <= bestAxis; axis++)

Not less interesting example with a cycle:

 public override void Draw() { .... foreach (var flame in m_thrust.Flames) { if (m_thrust.CubeGrid.Physics == null) continue; .... if (m_landingEffect != null) { m_landingEffect.Stop(true); m_landingEffect = null; --m_landingEffectCount; } continue; // <= .... if (m_landingEffect == null) continue; .... } }

PVS-Studio warning : V3020 An unconditional 'continue' within a loop. Sandbox.Game MyRenderComponentThrust.cs 109

The error lies in the fact that the 'continue' operator is outside the 'then'-branch of the' if 'operator, because of which it will always be executed. This means that all the code following this statement (more than 10 lines) will never be executed. The solution to the problem is obvious - it is necessary to make the 'continue' operator under the condition.

The remaining warnings


I repeat that in the articles I do not write out all the warnings that the analyzer issues on the source code of the project. It would take too much time to analyze all warnings, the article on volumes would become excessively large and it would be tiresome to read it. But you are probably wondering how many suspicious places in the code did the analyzer find? At the time of this writing, the statistics were as follows:


You should definitely study all the warnings of a high level of importance and at least look at the average level. Do not think that the warnings of a low level of importance do not contain anything interesting, just that more specific diagnostics are located at this level. In any case, it’s worthwhile to get acquainted with the list of these messages, since, perhaps, it contains exactly those specific diagnostics that are necessary for your project.

The benefits of static analysis and how to correctly use a static analyzer




Unfortunately, we have to deal with the fact that many people do not know how to correctly use a static analyzer.

The following variant of work is often considered to be acceptable: they downloaded the analyzer, checked the project before the release, corrected something, forgot about the analyzer. Oh, release soon! They remembered about the analyzer, checked the project, corrected something, forgot it.

This is the most incorrect use case. Errors made at the time of development are not immediately detected by the static analyzer, but rest in the code. Some of them are found by the compiler, some by the programmer himself, and another part by the testers. And only what remains after all this is found by the static analyzer, when they nevertheless decide to resort to its help. This requires a lot of effort from different people, and still there is a high chance of missing something. What is even worse - the longer the error is in the code, the more expensive it will eventually be fixed.

If the analyzer were used regularly, most of the errors would be corrected at the design stage, which would facilitate the task for both the programmer and the testers.

It is quite possible that the static analyzer will find too many errors, and then they simply will not edit. This could be avoided in 2 ways:


From the above, a simple conclusion follows - a static analyzer is a tool for regular use, not a one-time use. It is in this scenario of work that he fully reveals his potential, allowing him to correct errors at the earliest stages of their occurrence, when the cost of correction is still small.

Conclusion




To summarize, I will not talk about the quality of the source code, whether a project is good or bad - these are subjective concepts, everyone’s views may differ. Some impression you can make on the basis of the figures given in the article (the number of warnings) and code fragments. You will get a more complete picture by checking the project yourself and reviewing the messages, which I highly recommend to do. This will allow you to make a more detailed picture of the quality of the code, as well as to get a closer look at the static analyzer, the information on the use of which I tried to convey to you in this article.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. Unicorn in Space: Analyzing the Source Code of 'Space Engineers' .

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/277119/

More articles:


All Articles