chore: remove CONTRIBUTING.md and LESSONS_LEARNED.md from repository
These files were deleted as they are no longer relevant to the project's current state. CONTRIBUTING.md contained outdated contribution guidelines, and LESSONS_LEARNED.md held a postmortem analysis that is now archived elsewhere. The README.md was updated to reflect their removal from the directory listing.
This commit is contained in:
parent
4a9b4bdf3a
commit
3dacbb3ab0
@ -1,35 +0,0 @@
|
|||||||
# Contributing to Nimcheck
|
|
||||||
|
|
||||||
Thank you for considering contributing to Nimcheck.
|
|
||||||
|
|
||||||
## Getting Started
|
|
||||||
|
|
||||||
1. Ensure you have Nim >= 2.0.0 installed.
|
|
||||||
2. Fork and clone the repository.
|
|
||||||
3. Run the test suite to verify your environment:
|
|
||||||
|
|
||||||
```bash
|
|
||||||
make test
|
|
||||||
```
|
|
||||||
|
|
||||||
## Development Workflow
|
|
||||||
|
|
||||||
- Use `make lint` to check for warnings before submitting.
|
|
||||||
- Run `make test` to run the full test suite. All tests must pass.
|
|
||||||
- Follow the existing code style and conventions.
|
|
||||||
|
|
||||||
## Adding a New Language
|
|
||||||
|
|
||||||
See [LESSONS_LEARNED.md](LESSONS_LEARNED.md) Section 16 for a detailed checklist
|
|
||||||
covering tokenizer, validator, registration, tests, and documentation.
|
|
||||||
|
|
||||||
## Pull Requests
|
|
||||||
|
|
||||||
- Open an issue to discuss significant changes before investing time.
|
|
||||||
- Keep PRs focused on a single change.
|
|
||||||
- Ensure all tests pass and no warnings are introduced.
|
|
||||||
- Update documentation if the public API changes.
|
|
||||||
|
|
||||||
## Code of Conduct
|
|
||||||
|
|
||||||
Be respectful. Keep discussions constructive.
|
|
||||||
@ -1,695 +0,0 @@
|
|||||||
# Nimcheck Postmortem: Lessons Learned
|
|
||||||
|
|
||||||
A root-cause analysis of every bug, anti-pattern, and structural issue
|
|
||||||
encountered during the development of the Nimcheck validation framework.
|
|
||||||
|
|
||||||
**Date:** 2026-07-08
|
|
||||||
**Author:** AI-assisted analysis
|
|
||||||
**Project:** Nimcheck — multi-language source code validation in Nim
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Table of Contents
|
|
||||||
|
|
||||||
1. [Executive Summary](#1-executive-summary)
|
|
||||||
2. [Bug Taxonomy](#2-bug-taxonomy)
|
|
||||||
3. [Class 1: The `foundClosing` Copy-Paste Bug](#3-class-1-the-foundclosing-copy-paste-bug)
|
|
||||||
4. [Class 2: `result` Shadowing](#4-class-2-result-shadowing)
|
|
||||||
5. [Class 3: Import Bloat — Unused and Duplicate Imports](#5-class-3-import-bloat)
|
|
||||||
6. [Class 4: The Debug Import Dependency](#6-class-4-the-debug-import-dependency)
|
|
||||||
7. [Class 5: Silent `discard` Violations](#7-class-5-silent-discard-violations)
|
|
||||||
8. [Class 6: Named Parameter Syntax](#8-class-6-named-parameter-syntax)
|
|
||||||
9. [Class 7: Underscore-Prefix Identifiers](#9-class-7-underscore-prefix-identifiers)
|
|
||||||
10. [Class 8: Factory Registration Pattern — Side-Effect Imports](#10-class-8-factory-registration-pattern)
|
|
||||||
11. [Class 9: Missing `.gitignore` — Build Artifacts Risk](#11-class-9-missing-gitignore)
|
|
||||||
12. [Class 10: Repository Directory Name Mismatch](#12-class-10-repository-directory-name-mismatch)
|
|
||||||
13. [Root-Cause Analysis: Why These Bugs Happen](#13-root-cause-analysis)
|
|
||||||
14. [Systemic Anti-Patterns](#14-systemic-anti-patterns)
|
|
||||||
15. [Verification Protocol for Future Work](#15-verification-protocol)
|
|
||||||
16. [Checklist for New Tokenizer/Validator Modules](#16-checklist-for-new-modules)
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 1. Executive Summary
|
|
||||||
|
|
||||||
Nimcheck was developed by generating 7 tokenizers, 7 validators, and 3 config
|
|
||||||
validators from a common template. The template itself had bugs, and the copy-
|
|
||||||
paste propagation amplified every defect across every language. Additionally,
|
|
||||||
the codebase accumulated unused imports, shadowed variables, and dead code
|
|
||||||
because no compilation step was run between successive generations.
|
|
||||||
|
|
||||||
**Total bugs found and fixed:** 16 distinct classes, ~95 individual file hits.
|
|
||||||
**Categorization:**
|
|
||||||
|
|
||||||
| Class | Description | Files Affected | Root Cause |
|
|
||||||
|---|---|---|---|
|
|
||||||
| 1 | Missing `var foundClosing` declaration | 4 tokenizers | Buggy template |
|
|
||||||
| 2 | `result` shadowing | 1 (nimcheck.nim) | Nim implicit `result` vs. local `result` collision |
|
|
||||||
| 3 | Unused/duplicate imports | ~22 files | No cleanup, cargo-cult imports |
|
|
||||||
| 4 | Debug imports on non-debug files | 15+ files | Template included debug unconditionally |
|
|
||||||
| 5 | `discard` violations | 1 (html_tokenizer) | `advance()` returns char but was discarded |
|
|
||||||
| 6 | Named param syntax (`hint:` vs `hint =`) | 1 (config_validators) | Nim named-argument syntax confusion |
|
|
||||||
| 7 | Underscore-prefix identifiers | 1 (config_validators) | Nim doesn't allow `_` prefix |
|
|
||||||
| 8 | Zero-content `init_validators.nim` | 1 | Dead code left from architecture refactor |
|
|
||||||
| 9 | Missing `.gitignore` | 0 (repo-wide) | No `.gitignore` at repo init; build artifacts would be tracked |
|
|
||||||
| 10 | Directory name vs. project name mismatch | 1 (repo root) | Project directory `nimcheck` never renamed after project was renamed to `nimcheck` |
|
|
||||||
|
|
||||||
**250+ tests pass. Zero compiler warnings.**
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 2. Bug Taxonomy
|
|
||||||
|
|
||||||
Every bug class is ordered by how many files it infected (most widespread first).
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 3. Class 1: The `foundClosing` Copy-Paste Bug
|
|
||||||
|
|
||||||
### Severity: CRITICAL — would produce wrong compilation (undeclared identifier)
|
|
||||||
|
|
||||||
### Files Affected
|
|
||||||
- `src/nimcheck/tokenizers/nim_tokenizer.nim` (3 separate string contexts)
|
|
||||||
- `src/nimcheck/tokenizers/bash_tokenizer.nim` (double-quoted strings)
|
|
||||||
- `src/nimcheck/tokenizers/javascript_tokenizer.nim` (template literals)
|
|
||||||
- `src/nimcheck/tokenizers/python_tokenizer.nim` (triple-quoted, f-strings, regular strings)
|
|
||||||
- `src/nimcheck/tokenizers/jinja_tokenizer.nim` (removed a standalone `foundClosing = true` that declared nothing)
|
|
||||||
|
|
||||||
### The Pattern
|
|
||||||
Every string-parsing loop in every tokenizer follows the same pattern:
|
|
||||||
|
|
||||||
```
|
|
||||||
var foundClosing = false
|
|
||||||
while self.hasMore():
|
|
||||||
if <closing condition>:
|
|
||||||
...
|
|
||||||
foundClosing = true
|
|
||||||
break
|
|
||||||
else:
|
|
||||||
content.add(self.advance())
|
|
||||||
if not foundClosing:
|
|
||||||
<record error>
|
|
||||||
```
|
|
||||||
|
|
||||||
### What Went Wrong
|
|
||||||
In the **template** that the tokenizers were copied from, parts of the code
|
|
||||||
used `foundClosing` without a declaration. Specifically:
|
|
||||||
|
|
||||||
1. **`nim_tokenizer.nim`** — Three string loops (triple-quoted, regular, character)
|
|
||||||
each had `if not foundClosing:` and `foundClosing = true` but the `var foundClosing`
|
|
||||||
declaration was missing from the regular string loop. Additionally, a stray
|
|
||||||
`foundClosing = true` was present inside number-parsing code that had nothing
|
|
||||||
to do with strings — leftover from copy-paste.
|
|
||||||
|
|
||||||
2. **`python_tokenizer.nim`** — Triple-quoted string, f-string, and regular string
|
|
||||||
loops each had the same missing declaration. Duplicate `foundClosing = true`
|
|
||||||
lines were left in from partial template cleanup.
|
|
||||||
|
|
||||||
3. **`bash_tokenizer.nim`** — Double-quoted string loop missing declaration.
|
|
||||||
|
|
||||||
4. **`javascript_tokenizer.nim`** — Template literal loop missing declaration.
|
|
||||||
A duplicate `foundClosing = true` sat at the wrong indentation level.
|
|
||||||
|
|
||||||
5. **`jinja_tokenizer.nim`** — A line `foundClosing = true` existed without any
|
|
||||||
enclosing `var foundClosing` — the variable was never declared anywhere in
|
|
||||||
the file. Pure dead code.
|
|
||||||
|
|
||||||
### Prevention
|
|
||||||
- **Tokenize, don't replicate.** Write ONE string-parsing routine in the base
|
|
||||||
class (`TokenizerBase`) and call it from subclasses. Every tokenizer that
|
|
||||||
writes its own string parser invites copy-paste drift.
|
|
||||||
- **Unit test string parsing in isolation** in the base class.
|
|
||||||
- **Review diffs** when copying a template: every `var` declaration must have a
|
|
||||||
corresponding use, and every `foundClosing = true` must have a matching
|
|
||||||
`var foundClosing` in scope.
|
|
||||||
|
|
||||||
### Fix Applied
|
|
||||||
Added `var foundClosing = false` before each affected while loop. Removed
|
|
||||||
stray `foundClosing` references from contexts where they did not belong
|
|
||||||
(number parsing, wrong indentation levels). Changed `if not foundClosing:`
|
|
||||||
to `else:` in some cases for cleaner control flow.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 4. Class 2: `result` Shadowing
|
|
||||||
|
|
||||||
### Severity: CRITICAL — causes incorrect field access at runtime
|
|
||||||
|
|
||||||
### Files Affected
|
|
||||||
- `src/nimcheck.nim` (6+ locations across 4 functions)
|
|
||||||
|
|
||||||
### The Pattern
|
|
||||||
```nim
|
|
||||||
proc inspectSource*(source: string, ...): JsonNode =
|
|
||||||
let r = validateSource(source, flavor, options, sourcePath)
|
|
||||||
if result.valid or result.errors.len > 0: # BUG: 'result' is the JsonNode, not the ValidationResult
|
|
||||||
result.moduleInfo.toJson() # JsonNode has no .valid, .errors, or .moduleInfo
|
|
||||||
```
|
|
||||||
|
|
||||||
### What Went Wrong
|
|
||||||
Nim has an implicit `result` variable in every proc that returns a value.
|
|
||||||
It is automatically typed to the return type. Here, `inspectSource` returns
|
|
||||||
`JsonNode`, so `result` is a `JsonNode`. The local variable was named `r`
|
|
||||||
but the code accidentally referenced `result` (the implicit one), which
|
|
||||||
has no `.valid`, `.errors`, or `.moduleInfo` fields — those belong to
|
|
||||||
`ValidationResult`. The Nim compiler caught this because `JsonNode` has
|
|
||||||
no `.valid` field, but it took multiple compile-fix cycles because each
|
|
||||||
function had the same pattern.
|
|
||||||
|
|
||||||
### Functions Affected
|
|
||||||
- `inspectSource` (lines 201-205)
|
|
||||||
- `inspectFile` (lines 213-217)
|
|
||||||
- `reportSource` (lines 233-234)
|
|
||||||
- `reportFile` (lines 239-240)
|
|
||||||
- `main` CLI (lines 336-344)
|
|
||||||
|
|
||||||
### Prevention
|
|
||||||
- **Never name a local variable `r`** in a proc that returns something.
|
|
||||||
Name it `validationResult` or `vr` explicitly and there's no ambiguity.
|
|
||||||
- **Enable the `resultShadowed` warning** in the Nim config:
|
|
||||||
```
|
|
||||||
--warning[ResultShadowed]:on
|
|
||||||
```
|
|
||||||
- **Search for `result.`** after every refactor that changes variable names.
|
|
||||||
- **Compilation gate**: always compile after changing any proc that
|
|
||||||
uses both `result` and a local variable.
|
|
||||||
|
|
||||||
### Fix Applied
|
|
||||||
Replaced every `result.` reference with `r.` in the affected functions,
|
|
||||||
then verified by compiling.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 5. Class 3: Import Bloat
|
|
||||||
|
|
||||||
### Severity: LOW (warnings only) but HIGH in volume (~50 warnings)
|
|
||||||
|
|
||||||
### Files Affected
|
|
||||||
Every single `.nim` file in the project except `types.nim`.
|
|
||||||
|
|
||||||
### The Pattern
|
|
||||||
Two distinct sub-patterns:
|
|
||||||
|
|
||||||
#### 5a. Cargo-Cult Importing
|
|
||||||
Every tokenizer imported the same large set of standard library modules:
|
|
||||||
```nim
|
|
||||||
import std/[strutils, sets, strformat, tables, sets] # 'sets' listed TWICE
|
|
||||||
```
|
|
||||||
But the actual code in each tokenizer used maybe 2-3 of these. The rest were
|
|
||||||
copied from the first tokenizer that was ever written.
|
|
||||||
|
|
||||||
Additionally, many files had the import line in the form:
|
|
||||||
```nim
|
|
||||||
import std/[ sets, strformat, tables, sets] # note the spaces
|
|
||||||
```
|
|
||||||
which is syntactically valid but visually inconsistent, and the duplicate
|
|
||||||
`sets` went unnoticed.
|
|
||||||
|
|
||||||
#### 5b. Export-Everything in types.nim
|
|
||||||
```nim
|
|
||||||
export json, strutils, tables, times
|
|
||||||
```
|
|
||||||
This re-exports four standard library modules so that importing `types`
|
|
||||||
brings them all in. This is the root cause of why other files could import
|
|
||||||
just `./types` and still compile (the re-export cascaded). But then the
|
|
||||||
same files also imported `std/[...]` directly — producing unused-import
|
|
||||||
warnings.
|
|
||||||
|
|
||||||
### Why Import Bloat is Dangerous
|
|
||||||
Unused imports are not just cosmetic:
|
|
||||||
- They slow compile time.
|
|
||||||
- They can introduce name collisions.
|
|
||||||
- They mask real dependency analysis: you cannot tell at a glance what a
|
|
||||||
module actually needs.
|
|
||||||
- They spread when files are used as templates for new modules.
|
|
||||||
|
|
||||||
### Prevention
|
|
||||||
- **Use `--warning[UnusedImport]:on` during development.**
|
|
||||||
- **Remove `export stdlib`** from project modules. Each consumer should
|
|
||||||
import what it needs.
|
|
||||||
- **Clean imports as part of code review.** Every `import` line should be
|
|
||||||
justified.
|
|
||||||
- **Run `nim c --warning[UnusedImport]:on`** before merging.
|
|
||||||
|
|
||||||
### Fix Applied
|
|
||||||
Stripped each file to exactly the imports it uses. Added
|
|
||||||
`{.warning[UnusedImport]:off.}` pragmas where imports are conditionally used
|
|
||||||
(debug mode) to document the intent.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 6. Class 4: The Debug Import Dependency
|
|
||||||
|
|
||||||
### Severity: HIGH (every file imported debug whether it needed it or not)
|
|
||||||
|
|
||||||
### Files Affected
|
|
||||||
All 7 tokenizers, all 7 validators, config_validators, errors.nim,
|
|
||||||
detector.nim, validatorbase.nim, tokenizerbase.nim — 15+ files.
|
|
||||||
|
|
||||||
### The Pattern
|
|
||||||
Every module that might need debug logging included:
|
|
||||||
```nim
|
|
||||||
import ../core/types, ../core/tokenizerbase, ../core/debug
|
|
||||||
```
|
|
||||||
But `../core/debug` was only needed when `defined(nimcheckDebug)` was true.
|
|
||||||
The debug module provides `debugEnter`, `debugLeave`, `debugLog`, `debugError`,
|
|
||||||
and `debugToken` — none of which are used when debug mode is off.
|
|
||||||
|
|
||||||
However, removing the import entirely fails because the `when defined()`
|
|
||||||
blocks reference those procs. So the import IS needed for conditional use,
|
|
||||||
but the compiler sees "imported and not used" because the import is guarded
|
|
||||||
by a `when` block at the use site, not at the import site.
|
|
||||||
|
|
||||||
### Root Cause
|
|
||||||
The template was written as:
|
|
||||||
```nim
|
|
||||||
import std/[...]
|
|
||||||
import ../core/types, ../core/tokenizerbase, ../core/debug
|
|
||||||
```
|
|
||||||
And every file replicated this without considering whether it actually
|
|
||||||
called debug procs.
|
|
||||||
|
|
||||||
### Prevention
|
|
||||||
- **Use `{.warning[UnusedImport]:off.}`** on the import line, with a comment
|
|
||||||
explaining why.
|
|
||||||
- **Or, use a single import** in a common base module.
|
|
||||||
- **Better: use `debugEnter`/`debugLeave` ONLY in the tokenizerbase/validatorbase**
|
|
||||||
and let subclasses inherit the behavior.
|
|
||||||
|
|
||||||
### Fix Applied
|
|
||||||
Removed debug imports from files that don't use debug procs. Added warning
|
|
||||||
suppression pragmas with comments for files that conditionally use them.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 7. Class 5: Silent `discard` Violations
|
|
||||||
|
|
||||||
### Severity: MEDIUM (would fail compilation on stricter settings)
|
|
||||||
|
|
||||||
### Files Affected
|
|
||||||
- `src/nimcheck/tokenizers/html_tokenizer.nim` (line 87)
|
|
||||||
|
|
||||||
### What Went Wrong
|
|
||||||
```nim
|
|
||||||
self.advance()
|
|
||||||
```
|
|
||||||
`advance()` returns a `char`, but the return value was not used and not
|
|
||||||
`discard`ed. Nim flags this as "expression of type 'char' not used" — it's
|
|
||||||
not a hard error by default, but it is with `--warning[Discardable]:on`.
|
|
||||||
|
|
||||||
The same pattern exists in `tokenizerbase.nim` itself (lines 111, 113, etc.)
|
|
||||||
but those are correctly `discard self.advance()`. The html_tokenizer was
|
|
||||||
generated from a template where the `discard` was forgotten.
|
|
||||||
|
|
||||||
### Prevention
|
|
||||||
- **Use `--warning[UnusedResult]:on`** during development.
|
|
||||||
- **Always write `discard self.advance()`** instead of `self.advance()`
|
|
||||||
in all new tokenizer code.
|
|
||||||
- **Make `advance()` a `func` returning `char`** and mark it `.discardable`
|
|
||||||
so bare calls are allowed but the read-syntax is `discard`.
|
|
||||||
|
|
||||||
### Fix Applied
|
|
||||||
Changed `self.advance()` to `discard self.advance()`.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 8. Class 6: Named Parameter Syntax (`hint:` vs `hint =`)
|
|
||||||
|
|
||||||
### Severity: CRITICAL (compilation error)
|
|
||||||
|
|
||||||
### Files Affected
|
|
||||||
- `src/nimcheck/languages/config_validators.nim` (lines 113, 191)
|
|
||||||
|
|
||||||
### What Went Wrong
|
|
||||||
Nim uses two syntaxes for named arguments in procedure calls:
|
|
||||||
- **Correct:** `newValidationError(severity, message, code, position, range, hint = str)`
|
|
||||||
- **Incorrect:** `newValidationError(severity, message, code, position, range, hint: str)`
|
|
||||||
|
|
||||||
The colon syntax (`hint:`) is for proc *declaration* parameters and case
|
|
||||||
object fields. The equals sign (`hint =`) is for proc *call* arguments.
|
|
||||||
|
|
||||||
The config_validators.nim file used:
|
|
||||||
```nim
|
|
||||||
hint: "..."
|
|
||||||
```
|
|
||||||
in a proc *call*, which Nim interpreted as an invalid expression with the
|
|
||||||
identifier `hint` followed by a colon (invalid token in expression context).
|
|
||||||
|
|
||||||
### Why It Happened
|
|
||||||
The author was likely writing Nim code influenced by Python keyword argument
|
|
||||||
syntax (`hint=...` in Python, but `hint: ...` in some pseudocode contexts) or
|
|
||||||
TypeScript named parameter syntax (`{ hint: "..." }`).
|
|
||||||
|
|
||||||
### Prevention
|
|
||||||
- **Know the difference:** `param: Type` in declaration, `param = value` in call.
|
|
||||||
- **Run compilation** after every edit that touches function calls with named
|
|
||||||
arguments.
|
|
||||||
- **Pattern to grep for:** `hint:` in context of function calls (not type
|
|
||||||
definitions).
|
|
||||||
|
|
||||||
### Fix Applied
|
|
||||||
Changed `hint:` to `hint =` in the two function call sites.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 9. Class 7: Underscore-Prefix Identifiers
|
|
||||||
|
|
||||||
### Severity: CRITICAL (compilation error)
|
|
||||||
|
|
||||||
### Files Affected
|
|
||||||
- `src/nimcheck/languages/config_validators.nim` (line 110)
|
|
||||||
|
|
||||||
### What Went Wrong
|
|
||||||
```nim
|
|
||||||
var _indentStack: seq[int] = @[]
|
|
||||||
```
|
|
||||||
Nim does not allow identifiers starting with an underscore. The error message:
|
|
||||||
`invalid token: _ (\95)`
|
|
||||||
|
|
||||||
This is a Python convention (`_indentStack` means "private"), but Nim uses
|
|
||||||
`*` for export and has no `_` prefix convention. The `_` character alone
|
|
||||||
IS a valid identifier in Nim (it means "discard"), but `_indentStack` is not.
|
|
||||||
|
|
||||||
### Why It Happened
|
|
||||||
The author was writing Nim with Python naming conventions.
|
|
||||||
|
|
||||||
### Prevention
|
|
||||||
- **Know your language's identifier rules.** Nim: letters, digits, `_`
|
|
||||||
(but not starting with `_`). No `$` or `@` in identifiers.
|
|
||||||
- **Use `private*` or `hidden` naming** instead of underscore prefix.
|
|
||||||
- **Grep for `_\w`** (underscore followed by word character) to find violations.
|
|
||||||
|
|
||||||
### Fix Applied
|
|
||||||
Renamed `_indentStack` to `indentStack`.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 10. Class 8: Factory Registration Pattern — Side-Effect Imports
|
|
||||||
|
|
||||||
### Severity: MEDIUM (warnings only, but architectural concern)
|
|
||||||
|
|
||||||
### Files Affected
|
|
||||||
- `src/nimcheck/core/init_validators.nim` (dead, zero content)
|
|
||||||
- `src/nimcheck.nim` (imports 8 validator modules for their side effects)
|
|
||||||
- Every validator module (calls `init()` at module scope)
|
|
||||||
|
|
||||||
### The Pattern
|
|
||||||
Each language validator registers itself by calling `init()` at module scope
|
|
||||||
(line 149 of `nim_validator.nim`):
|
|
||||||
```nim
|
|
||||||
proc init*() =
|
|
||||||
registerValidator("nim", ...)
|
|
||||||
init() # side-effect at module load time
|
|
||||||
```
|
|
||||||
|
|
||||||
Then `nimcheck.nim` imports all validators:
|
|
||||||
```nim
|
|
||||||
import nimcheck/languages/nim_validator
|
|
||||||
import nimcheck/languages/bash_validator
|
|
||||||
# ... etc
|
|
||||||
```
|
|
||||||
|
|
||||||
This means importing a language validator triggers its registration
|
|
||||||
as a side effect. The Nim compiler warns about unused imports because
|
|
||||||
the imported module's symbols aren't directly referenced — only the
|
|
||||||
side effect matters.
|
|
||||||
|
|
||||||
### Why This Pattern is Fragile
|
|
||||||
1. **Ordering dependence:** All validators register at module load time,
|
|
||||||
which happens in import order. If two validators conflict, the second
|
|
||||||
silently overwrites the first.
|
|
||||||
2. **No dynamic discovery:** To add a new language, you must edit
|
|
||||||
`nimcheck.nim` to import it.
|
|
||||||
3. **Compiler warnings are unavoidable** without pragma suppression.
|
|
||||||
4. **The `init_validators.nim` file** was supposed to centralize this
|
|
||||||
but became a stub that does nothing — dead code.
|
|
||||||
|
|
||||||
### Better Approaches
|
|
||||||
- **Explicit registration** in a single function, not module-scope side effects.
|
|
||||||
Call `registerAll()` at startup.
|
|
||||||
- **Reflection-based discovery** using Nim's compile-time macro system
|
|
||||||
to enumerate registered validators.
|
|
||||||
- **Configuration-based registration** from a list of extension->validator
|
|
||||||
mappings.
|
|
||||||
|
|
||||||
### Mitigation Applied
|
|
||||||
Added `{.warning[UnusedImport]:off.}` pragma around the validator imports
|
|
||||||
in `nimcheck.nim` to suppress the warnings, with a comment explaining the
|
|
||||||
side-effect pattern.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 11. Class 9: Missing `.gitignore` -- Build Artifacts Risk
|
|
||||||
|
|
||||||
### Severity: MEDIUM (could accidentally commit large compiled binaries)
|
|
||||||
|
|
||||||
### Files Affected
|
|
||||||
- None directly, but the entire repository was at risk.
|
|
||||||
|
|
||||||
### The Pattern
|
|
||||||
The repository was initialised (`git init`) without a `.gitignore` file.
|
|
||||||
Build artifacts existed on disk:
|
|
||||||
```
|
|
||||||
bin/nimcheck 1.2 MB compiled binary
|
|
||||||
src/nimcheck.out 1.2 MB build output
|
|
||||||
tests/test_all 3.4 MB compiled test binary
|
|
||||||
tests/test_bash 1.2 MB compiled test binary
|
|
||||||
tests/test_nim 1.1 MB compiled test binary
|
|
||||||
tests/test_php 1.2 MB compiled test binary
|
|
||||||
dpc.log 400 B agent log
|
|
||||||
__pycache__/ 12 KB Python cache
|
|
||||||
```
|
|
||||||
|
|
||||||
Without a `.gitignore`, `git add .` would stage all of these, bloating
|
|
||||||
the repository with ~9.5 MB of binary artifacts that should never be
|
|
||||||
version-controlled.
|
|
||||||
|
|
||||||
### Root Cause
|
|
||||||
- No `.gitignore` was created at `git init` time.
|
|
||||||
- The build toolchain (Makefile, `nim c`) produces large compiled binaries
|
|
||||||
in the project tree (not in a separate `build/` or `nimcache/` directory),
|
|
||||||
making accidental commits likely.
|
|
||||||
- The `dpc.log` file is generated by the development tool and should never
|
|
||||||
be committed.
|
|
||||||
|
|
||||||
### Prevention
|
|
||||||
- **Create `.gitignore` at the same time as `git init`** -- make it part of the
|
|
||||||
project bootstrap checklist.
|
|
||||||
- **Run `git status` before every commit** to spot unexpected files.
|
|
||||||
- **Use `git add --dry-run`** to preview what would be staged.
|
|
||||||
- **Standard ignores to include** for any Nim project:
|
|
||||||
```
|
|
||||||
bin/
|
|
||||||
*.out
|
|
||||||
*.log
|
|
||||||
__pycache__/
|
|
||||||
*.pyc
|
|
||||||
*.o
|
|
||||||
*.exe
|
|
||||||
nimcache/
|
|
||||||
```
|
|
||||||
|
|
||||||
### Fix Applied
|
|
||||||
Created `.gitignore` excluding: `bin/`, `*.out`, `*.log`, `__pycache__/`,
|
|
||||||
`*.pyc`, compiled test binaries, OS junk files, and editor swap files.
|
|
||||||
Verified with `git add --dry-run` that only source files would be staged.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 12. Class 10: Repository Directory Name Mismatch
|
|
||||||
|
|
||||||
### Severity: LOW (cosmetic, but causes confusion)
|
|
||||||
|
|
||||||
### Files Affected
|
|
||||||
- Repository root directory
|
|
||||||
|
|
||||||
### The Pattern
|
|
||||||
The project was originally named **Validatrix** with a repository root directory
|
|
||||||
named `nimcheck`. All source code, the `.nimble` file, README, and every
|
|
||||||
reference used "validatrix" while the checkout directory was "nimcheck".
|
|
||||||
This mismatch between project name and directory name caused confusion.
|
|
||||||
|
|
||||||
### Root Cause
|
|
||||||
- The directory was created with one name and the project was renamed but
|
|
||||||
the directory rename was missed.
|
|
||||||
- No checklist item existed saying "rename the root directory when you rename
|
|
||||||
a project."
|
|
||||||
|
|
||||||
### Prevention
|
|
||||||
- **Add a project-bootstrap checklist** item: "Verify the project directory
|
|
||||||
name matches the project name."
|
|
||||||
- **When renaming a project**: rename the directory, update git remote,
|
|
||||||
update CI paths, update any scripts that reference the full path.
|
|
||||||
|
|
||||||
### Fix Applied
|
|
||||||
The project was eventually renamed from Validatrix to Nimcheck everywhere —
|
|
||||||
source code, documentation, `.nimble` file, and directory name now all match.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 13. Root-Cause Analysis
|
|
||||||
|
|
||||||
### Why Did All These Bugs Happen?
|
|
||||||
|
|
||||||
1. **No intermediate compilation.** Code was generated across multiple files
|
|
||||||
without running `nim c` between generations. If every file had been
|
|
||||||
compiled immediately after creation, the `foundClosing` bug would have
|
|
||||||
been caught in the first tokenizer, not after all 7 were written.
|
|
||||||
|
|
||||||
2. **Template entropy.** A single buggy template was copy-pasted with
|
|
||||||
modifications. Each copy introduced slight variations, but the template
|
|
||||||
bugs proliferated. Without diff review, each copy was assumed correct.
|
|
||||||
|
|
||||||
3. **Unused imports were invisible** because Nim by default doesn't warn
|
|
||||||
about unused imports in the compile output (they're in a separate warning
|
|
||||||
stream). The author didn't run `--warning[UnusedImport]:on`.
|
|
||||||
|
|
||||||
4. **No style guide or naming conventions.** The codebase mixes:
|
|
||||||
- Python underscore prefixes (`_indentStack`)
|
|
||||||
- CamelCase types (`ValidationResult`)
|
|
||||||
- PascalCase procs (`ValidateSource` vs `validateSource`)
|
|
||||||
This inconsistency led to confusion about what Nim allows.
|
|
||||||
|
|
||||||
5. **The `result` implicit variable** is uniquely Nim. Developers from
|
|
||||||
other languages (Python, JS, Rust) use `result` as a plain local variable
|
|
||||||
name and trip over the built-in.
|
|
||||||
|
|
||||||
6. **`hint:` / `hint =` confusion** stems from Nim's dual syntax for named
|
|
||||||
arguments (colon in declarations, equals in calls), which is unlike most
|
|
||||||
mainstream languages.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 14. Systemic Anti-Patterns
|
|
||||||
|
|
||||||
### 12a. Massive types.nim
|
|
||||||
`types.nim` is 456 lines with 19+ object types, 12+ toJson converter procs,
|
|
||||||
constructor helpers, and error code constants. This violates the Single
|
|
||||||
Responsibility Principle. Every file in the project imports it, creating
|
|
||||||
a tight coupling graph.
|
|
||||||
|
|
||||||
**Recommendation:** Split into:
|
|
||||||
- `types/flavors.nim` — LanguageFlavor enum
|
|
||||||
- `types/tokens.nim` — Token, TokenKind
|
|
||||||
- `types/errors.nim` — ValidationError, ErrorSeverity, error codes
|
|
||||||
- `types/results.nim` — ValidationResult, ValidationOptions
|
|
||||||
- `types/diagnostics.nim` — ModuleInfo, FunctionInfo, ClassInfo, etc.
|
|
||||||
|
|
||||||
### 12b. `export stdlib` in types.nim
|
|
||||||
```nim
|
|
||||||
export json, strutils, tables, times
|
|
||||||
```
|
|
||||||
This re-exports large standard library modules, making it impossible to
|
|
||||||
tell which module depends on what. Remove this and let consumers import
|
|
||||||
directly.
|
|
||||||
|
|
||||||
### 12c. Zero-Content `init_validators.nim`
|
|
||||||
This file exists, exports `initAllValidators*()`, but the proc body is
|
|
||||||
just `discard`. The validators register themselves on import instead.
|
|
||||||
Either remove the file or make it the single registration point.
|
|
||||||
|
|
||||||
### 12d. Same Pattern in Every Validator
|
|
||||||
Every validator has:
|
|
||||||
```nim
|
|
||||||
method analyzeTokens*(self: NimValidator) =
|
|
||||||
procCall ValidatorBase(self).analyzeTokens()
|
|
||||||
# ... language-specific analysis
|
|
||||||
```
|
|
||||||
The `procCall` is a fragile upcast. If the base class ever renames
|
|
||||||
`analyzeTokens`, all 7 subclasses silently break.
|
|
||||||
|
|
||||||
### 12e. No Abstract Error Codes in Base
|
|
||||||
The tokenizer base defines its own error strings rather than using the
|
|
||||||
constants from `types.nim` (`ErrUnexpectedToken`, `ErrUnclosedString`, etc.)
|
|
||||||
in some places, leading to inconsistent reporting.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 15. Verification Protocol
|
|
||||||
|
|
||||||
### Mandatory for Every Change
|
|
||||||
|
|
||||||
1. **Compile immediately** after any edit:
|
|
||||||
```bash
|
|
||||||
nim c --verbosity:0 src/nimcheck.nim
|
|
||||||
```
|
|
||||||
This catches: undeclared identifiers, type mismatches, invalid syntax.
|
|
||||||
|
|
||||||
2. **Check warnings explicitly:**
|
|
||||||
```bash
|
|
||||||
nim c --warning[UnusedImport]:on --warning[UnusedResult]:on src/nimcheck.nim 2>&1 | grep -E "Warning:|Hint:" | grep -v "Conf\|Link\|mm:\|Success"
|
|
||||||
```
|
|
||||||
|
|
||||||
3. **Run the full test suite:**
|
|
||||||
```bash
|
|
||||||
nim c -r tests/test_all.nim
|
|
||||||
```
|
|
||||||
Every test must pass before any work is done.
|
|
||||||
|
|
||||||
4. **On copy-paste: verify declarations exist in scope.**
|
|
||||||
Before writing a new tokenizer loop:
|
|
||||||
- Check every `var` declaration is present before its use
|
|
||||||
- Check every `foundClosing = true` has a `var foundClosing`
|
|
||||||
- Check every `discard` is explicit
|
|
||||||
|
|
||||||
### Optional but Recommended
|
|
||||||
|
|
||||||
- Add `nim check` to CI (type-checks without generating code, faster):
|
|
||||||
```bash
|
|
||||||
nim check src/nimcheck.nim
|
|
||||||
```
|
|
||||||
- Add `--warning[ResultShadowed]:on` to project config.
|
|
||||||
|
|
||||||
## 16. Checklist for New Tokenizer/Validator Modules
|
|
||||||
|
|
||||||
When adding a new language:
|
|
||||||
|
|
||||||
### Tokenizer
|
|
||||||
- [ ] Does it have `var foundClosing = false` before every string-parsing loop?
|
|
||||||
- [ ] Are all `self.advance()` calls preceded by `discard` when the return value is not used?
|
|
||||||
- [ ] Is `recordError` called with `hint =` (not `hint:`)?
|
|
||||||
- [ ] Are imports limited to what is actually used?
|
|
||||||
- [ ] Are all identifiers valid Nim (no `_` prefix)?
|
|
||||||
- [ ] Does it use `export *` or `{.used.}` for anything that needs it?
|
|
||||||
- [ ] Has it been compiled with `--warning[UnusedImport]:on`?
|
|
||||||
- [ ] Does the `method tokenize*` override compile without errors?
|
|
||||||
- [ ] Are error codes using constants from `types.nim` (e.g. `ErrUnclosedString`) rather than string literals?
|
|
||||||
|
|
||||||
### Validator
|
|
||||||
- [ ] Does the `init()` proc register correctly?
|
|
||||||
- [ ] Does importing the module produce any unused-import warnings?
|
|
||||||
- [ ] Is the `procCall ValidatorBase(self).analyzeTokens()` correct?
|
|
||||||
- [ ] Are all `toJson()` conversions using `.mapIt()` on sequences?
|
|
||||||
|
|
||||||
### Registration
|
|
||||||
- [ ] Is the validator added to the import list in `nimcheck.nim`?
|
|
||||||
- [ ] Are the imports wrapped with `{.warning[UnusedImport]:off.}`?
|
|
||||||
- [ ] Does `supportedFlavors()` include the new language?
|
|
||||||
|
|
||||||
### Testing
|
|
||||||
- [ ] Are there tests for: valid code, invalid code, empty source, null bytes, deep nesting?
|
|
||||||
- [ ] Does the exhaustive test module cover: unclosed strings, unclosed comments, mismatched brackets, binary data, unicode bombs?
|
|
||||||
- [ ] Are all 256+ existing tests still passing?
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Appendix: Complete Error Log
|
|
||||||
|
|
||||||
A chronological log of every error encountered:
|
|
||||||
|
|
||||||
| # | Error | File | Fix |
|
|
||||||
|---|---|---|---|
|
|
||||||
| 1 | `undeclared identifier: 'foundClosing'` | `nim_tokenizer.nim` | Added `var foundClosing = false` before triple-quoted, regular string loops |
|
|
||||||
| 2 | `undeclared identifier: 'foundClosing'` | `bash_tokenizer.nim` | Added `var foundClosing` before double-quoted string loop |
|
|
||||||
| 3 | `undeclared identifier: 'foundClosing'` | `python_tokenizer.nim` | Added `var foundClosing` before triple, f-string, and regular string loops |
|
|
||||||
| 4 | `undeclared identifier: 'foundClosing'` | `javascript_tokenizer.nim` | Added `var foundClosing` before template literal loop; removed duplicate assignment |
|
|
||||||
| 5 | `invalid token: _ (\95)` | `config_validators.nim:110` | Renamed `_indentStack` to `indentStack` |
|
|
||||||
| 6 | `invalid expression: hint:` | `config_validators.nim:113,191` | Changed `hint:` to `hint =` |
|
|
||||||
| 7 | `statement not allowed` / `invalid indentation` | `detector.nim:404` | Fixed indentation of `discard e` in except block |
|
|
||||||
| 8 | `type mismatch` (toHashSet) | `nim_tokenizer.nim:40` | Restored `sets` import |
|
|
||||||
| 9 | `undeclared field: 'valid' for type JsonNode` | `nimcheck.nim:202` | Changed `result.valid` to `r.valid` |
|
|
||||||
| 10 | `invalid indentation` | `detector.nim` | Re-fixed after multiple partial edits |
|
|
||||||
| 11 | `expression of type 'char' not used` | `html_tokenizer.nim:87` | Changed `self.advance()` to `discard self.advance()` |
|
|
||||||
| 12 | ~50 unused import warnings | All tokenizers, validators, core files | Stripped imports, added pragma suppressions |
|
|
||||||
| 13 | ~5 XDeclaredButNotUsed hints | `bash_validator.nim`, `config_validators.nim`, `nimcheck.nim` | Added `{.used.}` pragma or removed variable |
|
|
||||||
| 14 | ~3 DuplicateModuleImport hints | `nimcheck.nim`, 3 tokenizers | Deduplicated import lines |
|
|
||||||
@ -993,8 +993,6 @@ make rebuild # clean + build
|
|||||||
|-- Makefile
|
|-- Makefile
|
||||||
|-- nimcheck.nimble
|
|-- nimcheck.nimble
|
||||||
|-- README.md
|
|-- README.md
|
||||||
|-- LESSONS_LEARNED.md
|
|
||||||
|-- CONTRIBUTING.md
|
|
||||||
|-- LICENSE
|
|-- LICENSE
|
||||||
|-- bin/ # Build output
|
|-- bin/ # Build output
|
||||||
| `-- nimcheck # Compiled binary
|
| `-- nimcheck # Compiled binary
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user