The fastest reports in the wild west. And a handful of bugs to boot ...

Not only Microsoft has recently been putting open source code for its own projects - other companies are also following this trend. For us, the PVS-Studio developers, this is a great way to test the analyzer again, see what interesting it can find, and inform the project authors about it. Today we are looking inside the Fast Reports project.

What was checked?

FastReport is a report generator developed by Fast Reports . Written in C # and compatible with .NET Standard 2.0+. The source code of the project was recently posted on GitHub , from where it was uploaded for further analysis.

The reports support the use of text, images, lines, shapes, diagrams, tables, bar codes, etc. They can be single-page and multi-page, including, in addition to the data, a cover and a back page. XML, CSV, Json, MS SQL, MySql, Oracle, Postgres, MongoDB, Couchbase, RavenDB, SQLite can be used as data sources.

There are various ways to create report templates: from code; as an xml file; using an online designer or FastReport Designer Community Edition.
')
If necessary, the library can be downloaded as NuGet packages .

You can read more about the product features on the project's GitHub page .

There were no problems with the build of the project - I collected it from Visual Studio 2017, from where I checked it later using the PVS-Studio plugin.

PVS-Studio is a static analyzer that looks for errors in C, C ++, C #, Java code. When analyzing C # code, you can use the Visual Studio IDE analyzer using the PVS-Studio plugin for this, or you can check projects from the command line , for which the command-line utility PVS-Studio_Cmd.exe is used. If you wish, you can set up the analysis on the assembly server or pick up the analysis results to the SonarQube .

Well, let's see what was interesting this time.

Since the project is small, you should not count on a lot of typos and suspicious places. Let's look at what has been found, and even try to reproduce something in practice.

Ochepyatki and not only

I met the following method:

public override string ToString() { if (_value == null) return null; return this.String; } 

PVS-Studio warning : V3108 It is not recommended to return the 'null' from 'ToSting ()' method. Variant.cs 1519

Yes, returning null from the overridden ToString () method is not in itself an error, but it’s still a bad style. This is indicated, among other things, in the Microsoft documentation : Your ToString () should not return. Empty or a null string . Developers who do not expect to return null as a return value of ToString () can be unpleasantly surprised when an ArgumentNullException exception is thrown during the execution of the code below (provided that the extension method for IEnumerable <T> is called).

 Variant varObj = new Variant(); varObj.ToString().Contains(character); 

You can find fault with the fact that the example is synthetic, but the essence does not change.

Moreover, this code is commented as follows:

 /// <summary> /// Returns <see cref="String"/> property unless the value on the right /// is null. If the value on the right is null, returns "". /// </summary> /// <returns></returns> 

Oops. Returns "" instead of null .

We continue.

The library has a class FastString . Description: Fast alternative of StringBuilder . Actually, this class contains a field of type StringBuilder . The constructors of the FastString class call the Init method, which initializes the corresponding field.

Code of one of the designers:

 public FastString() { Init(initCapacity); } 

And the code of the Init method:

 private void Init(int iniCapacity) { sb = new StringBuilder(iniCapacity); //chars = new char[iniCapacity]; //capacity = iniCapacity; } 

If desired, you can access the sb field via the StringBuilder property:

 public StringBuilder StringBuilder { get { return sb; } } 

In total, FastString has 3 constructors:

 public FastString(); public FastString(int iniCapacity); public FastString(string initValue); 

I have already shown the body of the first designer, what the other two are doing to guess, I think, it’s also easy. And now, attention. Guess what output the following code:

 FastString fs = new FastString(256); Console.WriteLine(fs.StringBuilder.Capacity); 

Attention answer:

Suddenly? Let's look at the body of the corresponding constructor:

 public FastString(int iniCapacity) { Init(initCapacity); } 

Regular readers of our articles should already have an eye to find a problem here. At the eye analyzer (scent, logic, call it what you want) exactly planted, and he found the problem: V3117 Constructor parameter 'iniCapacity' is not used. FastString.cs 434

What a coincidence that the class code contains a constant field initCapacity , which is passed as an argument to the Init method instead of the iniCapacity constructor parameter ...

 private const int initCapacity = 32; 

When using similar names you need to be very, very attentive. Wherever there are errors associated with the use of similar names - projects in C, C ++, C #, Java - there were typos of this kind everywhere ...

By the way, about typos.

Let's make the following simple example and see how it works:

 static void Main(string[] args) { TextObject textObj = new TextObject(); textObj.ParagraphFormat = null; Console.WriteLine("Ok"); } 

As you may have guessed, the output will be different than the string “Ok” :)

How? So, for example:

The problem lies in the ParagraphFormat property and in the use of similar names:

 public ParagraphFormat ParagraphFormat { get { return paragraphFormat; } set { ParagraphFormat = value; } } 

PVS-Studio warning : V3110 Possible infinite recursion inside 'ParagraphFormat' property. TextObject.cs 281

The ParagraphFormat property is a wrapper over the paragraphFormat field. Moreover, its get property accessor is written correctly, but the set property accessor contains an annoying typo: instead of a field, an entry occurs in the same property, which causes recursion. Again the error associated with similar names.

Consider the following code snippet.

 public override Run Split(float availableWidth, out Run secondPart) { .... if (r.Width > availableWidth) { List<CharWithIndex> list = new List<CharWithIndex>(); for (int i = point; i < size; i++) list.Add(chars[i]); secondPart = new RunText(renderer, word, style, list, left + r.Width, charIndex); list.Clear(); for (int i = 0; i < point; i++) list.Add(chars[i]); r = new RunText(renderer, word, style, list, left, charIndex); return r; } else { List<CharWithIndex> list = new List<CharWithIndex>(); for (int i = point; i < size; i++) list.Add(chars[i]); secondPart = new RunText(renderer, word, style, list, left + r.Width, charIndex); list.Clear(); for (int i = 0; i < point; i++) list.Add(chars[i]); r = new RunText(renderer, word, style, list, left, charIndex); return r; } .... } 

PVS-Studio warning : V3004 The 'then' statement is equivalent to the 'else' statement. HtmlTextRenderer.cs 2092

A little copy-paste, and now the same actions will be performed regardless of the value of the expression r.Width> availableWidth . It is necessary either to remove the if statement , or to change the logic in one of the branches.

 public static string GetExpression(FindTextArgs args, bool skipStrings) { while (args.StartIndex < args.Text.Length) { if (!FindMatchingBrackets(args, skipStrings)) break; return args.FoundText; } return ""; } 

Analyzer warning : V3020 An unconditional 'return' within a loop. CodeUtils.cs 262

Due to the unconditional return operator for the above loop, no more than one iteration will be executed. Perhaps this code was obtained after refactoring, or maybe it is just an unusual way to do something that could be done without a loop.

 private int FindBarItem(string c) { for (int i = 0; i < tabelle_cb.Length; i++) { if (c == tabelle_cb[i].c) return i; } return -1; } internal override string GetPattern() { string result = tabelle_cb[FindBarItem("A")].data + "0"; foreach (char c in text) { int idx = FindBarItem(c.ToString()); result += tabelle_cb[idx].data + "0"; } result += tabelle_cb[FindBarItem("B")].data; return result; } 

PVS-Studio warning : V3106 Possible negative index value. The value of 'idx' index could reach -1. BarcodeCodabar.cs 70

Potentially dangerous code. The FindBarItem method can return -1 if it does not find the item passed as a parameter. In the calling code ( GetPattern method), this value is written to the idx variable and used as the index of the array tabelle_cb without prior checking. When accessing by index -1, an exception of type IndexOutOfRangeException will be generated.

We continue.

 protected override void Finish() { .... if (saveStreams) { FinishSaveStreams(); } else { if (singlePage) { if (saveStreams) { int fileIndex = GeneratedFiles.IndexOf(singlePageFileName); DoPageEnd(generatedStreams[fileIndex]); } else { .... } .... } .... } .... } 

PVS-Studio warning : V3022 Expression 'saveStreams' is always false. HTMLExport.cs 849

The given code with getting the fileIndex value and calling the DoPageEnd method will never be executed, since the result of the second saveStreams given in the code will always be false .

From the most interesting is, perhaps, everything (you did not wait for the article in the spirit of the analysis of Mono ?). There were other analyzer warnings, but they did not seem interesting enough to include them in the article (some always remain behind the scenes).

Knowledge of the project would be useful for analyzing them, so ideally the authors should look at these warnings on their own. These are warnings such as V3083 (a potentially dangerous invocation of event handlers), V3022 (the condition is always true / false (in this case, often because of methods that return one value)), V3072 , V3073 (working with IDisposable ) and others.

If any of this is irrelevant, you can:

• turn off diagnostics if the pattern it detects is exactly correct on the project;
• mark warnings as false positives if they are not very many;
• add warnings to the suppression database if you do not want to insert comments into the code. But then you need access to the base of suppression for all who work with the analyzer.

Conclusion

Despite the fact that the article came out small, I was pleased to “touch” the warnings of the analyzer with my hands - to see how what the analyzer curses is manifested in practice.

I wish success to the authors of the project, correcting the problems found and want to praise the step towards the open-source community!

I advise the rest to try the analyzer on your code and see what interesting things you can find.

All the best!

If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. The Fastest Reports in the Wild West - and a Handful of Bugs ...