Code quality hardening: nullable, analyzers, exception constructors

.editorconfig

@@ -0,0 +1,30 @@
+root = true
+
+[*]
+charset = utf-8
+insert_final_newline = true
+trim_trailing_whitespace = true
+indent_style = space
+indent_size = 4
+end_of_line = lf
+
+[*.{csproj,props,targets,json,yml,yaml}]
+indent_size = 2
+
+[*.cs]
+# --- Analyzer rule severities ---------------------------------------------
+# GenerateDocumentationFile turns on CS1591 for every undocumented public
+# member. Documenting the whole public surface is a gradual effort, so keep it
+# quiet for now and re-enable when the API is fully documented. The XML doc file
+# is still produced for the docs that DO exist (e.g. /// <exception> comments).
+dotnet_diagnostic.CS1591.severity = none
+
+# Analyzers should highlight real problems without failing local builds while
+# the backlog is triaged; promote individual rules (or set
+# TreatWarningsAsErrors) once each is clean.
+dotnet_analyzer_diagnostic.category-Style.severity = suggestion
+
+# Test method names deliberately use underscores (Should_do_x); that's a
+# readability convention here, not a naming-guideline violation.
+[test/**.cs]
+dotnet_diagnostic.CA1707.severity = none

Directory.Build.props

@@ -0,0 +1,25 @@
+<Project>
+
+  <!-- Shared defaults for every project in the repo. -->
+  <PropertyGroup>
+    <LangVersion>latest</LangVersion>
+    <!-- Roslyn/FxCop-style analysis in the build (reviewer recommendation #5).
+         AnalysisMode=All opts in to the full CA rule set, not just the minimal
+         default that ships enabled for SDK projects. -->
+    <EnableNETAnalyzers>true</EnableNETAnalyzers>
+    <AnalysisLevel>latest</AnalysisLevel>
+    <AnalysisMode>All</AnalysisMode>
+    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
+  </PropertyGroup>
+
+  <!-- Library-only settings. Scoped to the shipping assembly so the test and
+       benchmark projects don't drown in nullable/missing-doc warnings. -->
+  <PropertyGroup Condition="'$(MSBuildProjectName)' == 'DbfDataReader'">
+    <!-- Nullable reference types (reviewer recommendation #1). -->
+    <Nullable>enable</Nullable>
+    <!-- Emit XML docs into the package so downstream consumers get IntelliSense
+         and the /// <exception> docs from recommendation #4. -->
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+  </PropertyGroup>
+
+</Project>

docs/code-quality-hardening.md

@@ -0,0 +1,124 @@
+# Code Quality Hardening Plan
+
+Tracks the work from @daiplusplus's review on
+[PR #285](https://github.com/yellowfeather/DbfDataReader/pull/285#issuecomment-4952711673):
+enable nullable reference types, SDK-style multitargeting, exception
+documentation, and full Roslyn/FxCop analysis for a library with downstream
+consumers.
+
+**Status legend:** ✅ done · 🚧 in progress · ⬜ not started
+
+---
+
+## Phase 0 — Infrastructure (✅ done)
+
+Groundwork so warnings surface without blocking the build.
+
+- [x] Add `Directory.Build.props` — analyzers repo-wide (`AnalysisMode=All`,
+      `AnalysisLevel=latest`, `EnforceCodeStyleInBuild`); `Nullable=enable` and
+      `GenerateDocumentationFile=true` scoped to the `DbfDataReader` library.
+- [x] Add `.editorconfig` — formatting conventions; `CS1591` silenced (gradual
+      doc effort); style diagnostics as `suggestion`; `CA1707` silenced under
+      `test/**` (underscore test names are intentional).
+- [x] Fix empty/incomplete exception classes for **CA1032** —
+      `DbfFileFormatException`, `CdxException`, `SqlParseException` now expose the
+      standard constructors.
+- [x] Confirm baseline: solution builds with **0 errors**; ~442 warnings on the
+      library (CA1032 cleared).
+
+Already in place before the review (no action needed):
+- [x] SDK-style `.csproj` with multitargeting (`net10.0;netstandard2.1`).
+
+---
+
+## Phase 1 — Nullable reference types (🚧 in progress)
+
+~305 nullable warnings on the library, mostly on the public API surface. Do this
+before the analyzer pass — it collapses most of the CA1062 count. Work
+file-group by file-group so each PR is reviewable.
+
+Progress: core read path + value types annotated (net10.0 nullable warnings
+326 → 284), then the ADO.NET reader surface (`DbfDataReader`, `DbfDbConnection`,
+`DbfDbParameter`, `DbfDbParameterCollection`, `SchemaTableBuilder`) — those files
+are now nullable-clean. `DbfDbCommand` and the connection-string builders are
+deferred to the query-engine slice: their parameter-value nullability threads
+into `QueryPlanner`/`SqlExpressionEvaluator`, so they're best done together.
+Remaining warnings live in the query engine, CDX index, `DbfDbCommand`, and the
+connection-string builders.
+
+Warning inventory (per-build, `net10.0`):
+
+| Rule | Count | Meaning |
+|------|-------|---------|
+| CS8603 | 104 | Possible null reference return |
+| CS8625 | 94 | Cannot convert null literal to non-nullable reference |
+| CS8618 | 58 | Non-nullable field/property uninitialized on exit from ctor |
+| CS8765 | 28 | Nullability of override/interface parameter mismatch |
+| CS8604 | 20 | Possible null reference argument |
+| CS8600 | 14 | Converting null literal or possible null to non-nullable |
+| CS8601 | 6 | Possible null reference assignment |
+| CS8602 | 2 | Dereference of a possibly null reference |
+
+Suggested order:
+- [x] Core read path — `DbfTable`, `DbfRecord`, `DbfHeader`, `DbfColumn`, memo readers
+- [x] Value types — `DbfValue*`
+- [x] ADO.NET reader surface — `DbfDataReader`, `DbfDbConnection`, `DbfDbParameter`, `DbfDbParameterCollection`, `SchemaTableBuilder`
+- [ ] Query engine — `Query/*` (parser, planner, translator, evaluator, readers) **+ `DbfDbCommand` + connection-string builders** (parameter-value nullability threads through here)
+- [ ] CDX index — `Cdx/*`
+- [ ] Remove now-redundant manual null-checks flagged as dead by CA1508
+
+---
+
+## Phase 2 — Analyzer (CA) cleanup (⬜ not started)
+
+~137 analyzer warnings. Expect CA1062 to shrink dramatically after Phase 1.
+Group by theme; decide keep-vs-suppress per rule in `.editorconfig`.
+
+| Rule | Count | Theme | Likely action |
+|------|-------|-------|---------------|
+| CA1062 | 36 | Validate args non-null | Mostly resolved by nullable; `ThrowIfNull` the rest |
+| CA1510/CA1512 | 20 | Use `ArgumentNullException.ThrowIfNull` / `ThrowIf*` | Mechanical fix |
+| CA1307/CA1305 | 18 | Culture / `StringComparison` on string ops | Fix — correctness for locale-sensitive data |
+| CA1859 | 14 | Use concrete types for perf | Fix where hot; judgement call |
+| CA1051 | 8 | Do not declare visible instance fields | Review |
+| CA2249 | 6 | Prefer `string.Contains` over `IndexOf` | Mechanical |
+| CA2201 | 6 | Do not raise reserved exception types (`IndexOutOfRangeException`) | Fix |
+| CA1010 | 6 | Collections should implement generic interface | Review |
+| CA2100 | 2 | Review SQL string for injection | Review/annotate |
+| Others | ~30 | CA1720, CA1065, CA1002, CA2227, CA1870, CA1865, CA1861, CA1819, CA1724, CA1721, CA1508, CA1028, CA1008 | Triage individually |
+
+- [ ] Mechanical fixes (CA1510/CA1512/CA2249/CA1865)
+- [ ] Correctness fixes (CA1307/CA1305/CA2201/CA2100)
+- [ ] API/design review (CA1051/CA1002/CA1010/CA1819/CA2227/CA1724/CA1721)
+- [ ] Perf fixes where they matter (CA1859/CA1861/CA1870)
+- [ ] Record deliberate suppressions with justification in `.editorconfig`
+
+---
+
+## Phase 3 — Exception documentation (⬜ not started)
+
+Recommendation #4: `/// <exception>` comments so consumers know what to catch.
+`GenerateDocumentationFile` is already on; `CS1591` is currently silenced.
+
+- [ ] Document thrown exceptions on all public entry points (constructors,
+      `Read`/`ReadAsync`, `Open`, query execution, connection-string parsing)
+- [ ] Add `<summary>` docs to the public API
+- [ ] Re-enable `CS1591` once the public surface is documented
+
+---
+
+## Phase 4 — Enforce in CI (⬜ not started)
+
+Lock the gains in so regressions can't creep back.
+
+- [ ] Set `TreatWarningsAsErrors=true` for rules that are already clean
+      (per-rule via `.editorconfig`, or globally once Phases 1–2 land)
+- [ ] Confirm the CI build fails on new warnings
+
+---
+
+## Out of scope / optional
+
+- **`readonly struct` refinement types** (recommendation #3) — large effort, the
+  reviewer flagged it as tedious. Revisit as a separate initiative if desired,
+  e.g. for field offsets / record positions / encoding identifiers.

src/DbfDataReader/BinaryReaderExtensions.cs

@@ -35,7 +35,7 @@ public static uint ReadBigEndianUInt32(this BinaryReader binaryReader)
             return BitConverter.ToUInt32(bytes, 0);
         }
 
-        public static string ReadString(this BinaryReader binaryReader, int fieldLength, Encoding encoding)
+        public static string? ReadString(this BinaryReader binaryReader, int fieldLength, Encoding encoding)
         {
             var chars = binaryReader.ReadBytes(fieldLength);
             if ((chars == null) || (chars.Length == 0))

src/DbfDataReader/Cdx/CdxException.cs

@@ -4,6 +4,20 @@ namespace DbfDataReader.Cdx
 {
     public class CdxException : Exception
     {
+        public CdxException()
+        {
+        }
+
+        public CdxException(string message)
+            : base(message)
+        {
+        }
+
+        public CdxException(string message, Exception innerException)
+            : base(message, innerException)
+        {
+        }
+
         public CdxException(CdxErrorCode code)
             : base($"Invalid CDX index file: {code}")
         {

src/DbfDataReader/DbfColumn.cs

@@ -53,8 +53,9 @@ private void Read(ReadOnlySpan<byte> bytes, Encoding encoding)
                 DecimalCount = decimalCount;
             }
 
-            DataType = GetDataType(ColumnType);
-            DataTypeName = DataType.ToString();
+            var dataType = GetDataType(ColumnType);
+            DataType = dataType;
+            DataTypeName = dataType.ToString();
 
             // skip the reserved bytes:
             // - Int16: reserved1

src/DbfDataReader/DbfDataReader.cs

@@ -54,8 +54,8 @@ public override void Close()
             }
             finally
             {
-                DbfTable = null;
-                DbfRecord = null;
+                DbfTable = null!;
+                DbfRecord = null!;
             }
         }
 
@@ -69,9 +69,9 @@ protected override void Dispose(bool disposing)
             }
         }
 
-        public DbfRecord ReadRecord()
+        public DbfRecord? ReadRecord()
         {
-            DbfRecord dbfRecord;
+            DbfRecord? dbfRecord;
             bool skip;
             do
             {
@@ -105,7 +105,7 @@ public override byte GetByte(int ordinal)
             return DbfRecord.GetStructValue<byte>(ordinal);
         }
 
-        public override long GetBytes(int ordinal, long dataOffset, byte[] buffer, int bufferOffset, int length)
+        public override long GetBytes(int ordinal, long dataOffset, byte[]? buffer, int bufferOffset, int length)
         {
             throw new NotImplementedException();
         }
@@ -115,7 +115,7 @@ public override char GetChar(int ordinal)
             return DbfRecord.GetStructValue<char>(ordinal);
         }
 
-        public override long GetChars(int ordinal, long dataOffset, char[] buffer, int bufferOffset, int length)
+        public override long GetChars(int ordinal, long dataOffset, char[]? buffer, int bufferOffset, int length)
         {
             throw new NotImplementedException();
         }
@@ -261,12 +261,14 @@ public override int GetValues(object[] values)
 
         public override object GetValue(int ordinal)
         {
-            return DbfRecord.GetValue(ordinal);
+            // DbDataReader declares a non-null return, but this reader has always
+            // surfaced a null field value as null rather than DBNull; preserve that.
+            return DbfRecord.GetValue(ordinal)!;
         }
 
         public override string GetString(int ordinal)
         {
-            return DbfRecord.GetStringValue(ordinal);
+            return DbfRecord.GetStringValue(ordinal)!;
         }
 
         public override int GetOrdinal(string name)
@@ -291,7 +293,7 @@ public override int GetOrdinal(string name)
         public override string GetName(int ordinal)
         {
             var dbfColumn = DbfTable.Columns[ordinal];
-            return dbfColumn.ColumnName;
+            return dbfColumn.ColumnName!;
         }
 
         public override long GetInt64(int ordinal)
@@ -321,12 +323,12 @@ public override float GetFloat(int ordinal)
 
         public override Type GetFieldType(int ordinal)
         {
-            return DbfRecord.GetFieldType(ordinal);
+            return DbfRecord.GetFieldType(ordinal)!;
         }
 
         public ReadOnlyCollection<DbColumn> GetColumnSchema()
         {
-            var columns = DbfTable.Columns.Select(c => c as DbColumn).ToList();
+            var columns = DbfTable.Columns.Select(c => (DbColumn)c).ToList();
             return columns.AsReadOnly();
         }
 

src/DbfDataReader/DbfDataReaderOptions.cs

@@ -14,7 +14,7 @@ public DbfDataReaderOptions()
         }
 
         public bool SkipDeletedRecords { get; set; }
-        public Encoding Encoding { get; set; }
+        public Encoding? Encoding { get; set; }
         public StringTrimmingOption StringTrimming { get; set; }
         public bool ReadFloatsAsDecimals { get; set; }
 

src/DbfDataReader/DbfDbConnection.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Data;
 using System.Data.Common;
+using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Text;
 using System.Threading;
@@ -29,7 +30,17 @@ public DbfDbConnection(string dataSource, string serverVersion)
 
         public DbfDataReaderOptions Options { get; private set; } = new DbfDataReaderOptions();
 
-        public override string ConnectionString { get; set; }
+        private string _connectionString = string.Empty;
+
+        // base declares [AllowNull] on the setter; store a non-null value so the getter
+        // never returns null
+        [AllowNull]
+        public override string ConnectionString
+        {
+            get => _connectionString;
+            set => _connectionString = value ?? string.Empty;
+        }
+
         public override string DataSource { get; }
         public override string ServerVersion { get; }
 
@@ -104,7 +115,7 @@ protected override DbCommand CreateDbCommand()
         // Dapper-style typed queries: rows are mapped to T's settable properties by
         // column name (or to T itself for single-column scalar queries), and param's
         // properties become named parameters
-        public List<T> Query<T>(string sql, object param = null)
+        public List<T> Query<T>(string sql, object? param = null)
         {
             using (var command = CreateTypedCommand(sql, param))
             using (var reader = command.ExecuteReader())
@@ -123,7 +134,7 @@ public List<T> Query<T>(string sql, object param = null)
             }
         }
 
-        public async Task<List<T>> QueryAsync<T>(string sql, object param = null,
+        public async Task<List<T>> QueryAsync<T>(string sql, object? param = null,
             CancellationToken cancellationToken = default)
         {
             using (var command = CreateTypedCommand(sql, param))
@@ -143,7 +154,7 @@ public async Task<List<T>> QueryAsync<T>(string sql, object param = null,
             }
         }
 
-        public T QueryFirstOrDefault<T>(string sql, object param = null)
+        public T? QueryFirstOrDefault<T>(string sql, object? param = null)
         {
             using (var command = CreateTypedCommand(sql, param))
             using (var reader = command.ExecuteReader())
@@ -158,7 +169,7 @@ public T QueryFirstOrDefault<T>(string sql, object param = null)
             }
         }
 
-        private DbfDbCommand CreateTypedCommand(string sql, object param)
+        private DbfDbCommand CreateTypedCommand(string sql, object? param)
         {
             var command = (DbfDbCommand)CreateCommand();
             command.CommandText = sql;