From 76dd8eacb002d0c182476adf94c30c49743de394 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Apr 2026 20:33:41 +0000 Subject: [PATCH] compiler: fix three scoping bugs; war: revert all local/param workarounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related scoping bugs from examples/war/COMPILER_BUGS.md, all fixed in one pass because they're different layer manifestations of the same "flat global namespace" problem: ## §3: function-local `var` declarations lived in one namespace `src/analyzer/mod.rs::register_var` inserted every `var` it saw — top-level, state-local, AND function-body local — into the same `self.symbols: HashMap`. Two different functions declaring `var i` collided on E0501, which is why every local in war/*.ne had a function-prefix like `dfa_card` or `dwp_px`. Fix: add a `current_scope_prefix: Option` to the Analyzer, set it to `Some("")` when checking a function body (or `Some("Title__frame")` for state handler bodies), and have `register_var` store the declaration under an internal key `"__local__{prefix}__{name}"`. New `resolve_symbol` / `resolve_key` helpers try the scope-qualified key first and fall back to the bare key for globals / consts / enum variants / state-level vars / function names. Every existing `self.symbols.get(name)` inside body-checking code was swapped over. Two `var i` declarations inside the SAME function body still collide with E0501 — we scoped per function body, not per nested block. Per-block scoping would require live-range analysis to reuse RAM slots. ## §1b: same-named params across functions shared VarIds `src/ir/lowering.rs::get_or_create_var` looked up names in a single global `var_map`, so two functions both with a `card: u8` parameter resolved to the same `VarId`. Whichever function was lowered last won the zero-page slot mapping, silently rerouting the other function's param reads to the wrong slot. Fix: the IR lowerer now mirrors the analyzer's scope logic. `LoweringContext` gains a `current_scope_prefix` field that gets set in `lower_function` / `lower_handler`, and `get_or_create_var` uses a new `scoped_key` helper that prepends `"__local__{prefix}__"` when the qualified key exists in `var_map` or `var_types`. Each function's parameters and locals therefore get distinct VarIds, and the codegen's `var_addrs` map naturally has no collisions. ## §2: param transport slots $04-$07 clobbered across nested JSRs Parameters were passed AND kept in `$04-$07` for the lifetime of a function. Any nested call overwrote those slots with its own arguments, so the caller's params were silently corrupted as soon as it invoked anything. Every war helper that took params and called other helpers (draw_card_face, push_back_a, etc) snapshotted its params into fresh locals at the top of the body. Fix: in `codegen/ir_codegen.rs::IrCodeGen::new`, every function-local — including parameters — now gets a dedicated per-function RAM slot at `$0300+`. Parameters are still passed via the zero-page transport slots `$04-$07` as the calling convention, but `gen_function` now emits a **prologue** at every function entry: LDA $04 STA LDA $05 STA ... etc, up to 4 ... By the time the body runs, every parameter lives in the function's dedicated RAM slot, so any nested call can freely clobber $04-$07 (writing its own arguments there) without corrupting the caller's saved parameters. Costs 4 LDA/STA pairs (≈ 20 bytes of ROM, 16 cycles) at every function entry — worth it to make the calling convention sound. ## War cleanup With all three fixes in place, every workaround prefix in `examples/war/*.ne` is gone: - `card_rank(card)` instead of `card_rank(crk_c)` — bug #1b - `compare_cards(a, b)` instead of `compare_cards(cmp_a, cmp_b)` - `push_back_a(card)` instead of `push_back_a(pba_in)` — bug #1b - `var card: u8 = draw_front_a()` in bury_from_* — bug #3 - `var i: u8 = 0` freely in multiple functions — bug #3 - `fun push_back_a(card)` body no longer snapshots `card` into `pba_card` before calling wrap52 — bug #2 - `fun draw_card_face` body no longer snapshots x/y/card into locals before calling card_rank/card_suit — bug #2 - `draw_word_player` steps its own x without needing a `dwp_px` accumulator to avoid the `x + N` arg compilation quirk — that quirk was a downstream symptom of bug #2 and is also gone The source is now about 300 lines shorter and significantly more readable. ## Regression tests Seven new tests nail these bugs down: - `analyzer::tests::analyze_allows_same_local_name_in_two_functions` - `analyzer::tests::analyze_allows_same_param_name_in_two_functions` - `analyzer::tests::analyze_allows_same_local_name_in_two_state_handlers` - `analyzer::tests::analyze_still_rejects_duplicate_local_in_same_function` - `codegen::ir_codegen::gen_function_prologue_spills_params_to_local_ram` Plus the four param-arity tests from the earlier E0506 fix and the wide_hi-leak regression test from the previous compiler fix. Total suite: 591 unit tests, all passing. ## Golden drift The prologue change adds a few cycles to every function entry, which shifts NMI sampling by a handful of cycles and flips the audio-hash of any example that plays sfx or music (platformer, war). `arrays_and_functions.png` also picks up a 1-pixel shift in its enemy positions due to the same timing drift. All three golden updates are pure "compiler produces different but functionally-identical output" — no game behavior changed. ## What's still open in COMPILER_BUGS.md - §4: 8-sprites-per-scanline hardware limit is invisible to user code. A static analyzer hint could help; deferred. - §5: `inline` keyword is silently declined for short functions that the optimizer's inliner doesn't recognize (it only removes empty functions). Deferred pending a real single-return-expression inlining pass. https://claude.ai/code/session_0143dTgh3UeRrtfHgQwzcv5z --- examples/arrays_and_functions.nes | Bin 24592 -> 24592 bytes examples/coin_cavern.nes | Bin 24592 -> 24592 bytes examples/function_chain.nes | Bin 24592 -> 24592 bytes examples/inline_asm_demo.nes | Bin 24592 -> 24592 bytes examples/loop_break_continue.nes | Bin 24592 -> 24592 bytes examples/mmc1_banked.nes | Bin 57360 -> 57360 bytes examples/platformer.nes | Bin 24592 -> 24592 bytes examples/war.nes | Bin 24592 -> 24592 bytes examples/war/COMPILER_BUGS.md | 125 ++++++--- examples/war/compare.ne | 24 +- examples/war/deal_state.ne | 24 +- examples/war/deck.ne | 125 ++++----- examples/war/play_state.ne | 73 ++--- examples/war/render.ne | 182 +++++------- examples/war/state.ne | 1 - examples/war/victory_state.ne | 12 +- src/analyzer/mod.rs | 259 +++++++++++++----- src/analyzer/tests.rs | 100 +++++++ src/codegen/ir_codegen.rs | 143 ++++++++-- src/ir/lowering.rs | 102 +++++-- .../emulator/goldens/arrays_and_functions.png | Bin 1277 -> 1277 bytes tests/emulator/goldens/platformer.audio.hash | 2 +- tests/emulator/goldens/war.audio.hash | 2 +- 23 files changed, 762 insertions(+), 412 deletions(-) diff --git a/examples/arrays_and_functions.nes b/examples/arrays_and_functions.nes index 71509e643f604e2eaf35d9bc62d920d7b819514d..185c76d5e9c091c805d864c5ae039525c005b851 100644 GIT binary patch delta 334 zcmZ|KKTE?v7{~EkeizN3wrSI)W2gES92^`RGj?$+y6EhxD3luy0$p4j4@}{KLN`H3 z+9^Y;FTugFLSBG__mpOIy1VavxQELMkriTbEgt16y(o>;K8&XWkdIA(I#_tQliroN zs3irM*gUZ)WE6oyY@!A|GxBXZ{fR7$>>BQIi*Xy~NOxe4bqs1jN$Nm(!-rCLL2alL zN%0}{KwVUR#*=saXrFq``FHjqF2@NdK_&O%YL$ymr19FO`Mbp6R;VfO&GbdG<5+zi zev(;T2V8TfxpttQD4E^6gVDUyeN>|6(cAy&n>&&T((&@!-JN+iYo~Z!#^+-A2e<@r A>Hq)$ delta 318 zcmbPmfN{bB#tq4g^`{Q{xEx%{a+LK0=LXi53_j@xS2BKJUdo!Vlm&?3!gU~Fwxui< zPk}P57EghaY^|&c`3HMBfQnmLma;>5oXl%Em``#{02#Fjq(^uyCyajxZ1hUTgp~{l zix^h^o6ErTVzLaATRj)^S}x|3EMQF^q$?yTB zQx+ttv{XVmI_Lw3bhJ86FPYZtVL(C1Cs)y@8m=#G36AH3eBa0CsV*u#UQTK iGm&GWOGTk(v~8ZnRA4)K12a3T>tTiit(*BB7!?3i08nQD diff --git a/examples/function_chain.nes b/examples/function_chain.nes index 6471e4862ef9cbed09611aa8bc86d9faa03872a6..6af9715652ea907bc706737fe4816f464ab04721 100644 GIT binary patch delta 226 zcmXYpp^gGU42IkJcMcYDx9WmKKoAs{LyA;BVcP^?4q z0AwZZ+=Wh{qG|u{Yx9!xk`FzepZxgS@1u<12-W2gAFR-L;DsNMOR8b2l;IeeL6zhI zPXe}*XyOc(QK##J1vsFp-#->XYD%qmldYH|7|S+KW-hmx9Ka&LdhIa7 L0kO^e4vY!_EW=zy diff --git a/examples/mmc1_banked.nes b/examples/mmc1_banked.nes index e5e1109abec7da885767539548b1e581860ab6c2..fbeb3adeb79ef171aba79f75cced85cdc03c73b6 100644 GIT binary patch delta 231 zcmbPmfO*0J<_(dJq3aGVW%%%CB||I2Qs$Ek7npr^A3Vtjr1B5>cpqHK(#ycSl(mxlGKh0!tY+nLbD?6*$TCffKCe@8sW&(i)P7fciOBGPVk@WNj5vs5=N$ z@eHcM6ePz5lw(#%nrzpUBexbqF{7pePz3`}#kzx&?VD6KziHyPWwkuaaG-KC{{cn? E03sb(f&c&j delta 219 zcmbPmfO*0J<_(dJZu1W=W%%%CB||I2Qs$EkAA(OZUSRe~IOOAfa4Cz&Qr1?c424gJ z5|#>|WcmAHY4W;8TlwUJy$sAt1tfAAm|F#w zGHNn?kXR~klIa5{Sk1f1Tuss%T!(=AIaV^Z3a?~s6;jAQ2vqS5szMDU#|4yQRtTQ# t+ms_W4?{7drUFm}15m~MgOmN5R5t%^;ilMi~3H#t9K{K&hK|C>p+HMDFG#GklV)ZuF}q~!7>5s3LyS5H_D||${l)NZr?X$*E4Q6 zuoXMZP||IOw9w>&-3}dgva}h@BECsAgm1TEY62b5Va&Ev610KtipHe!|`37_-vQV$4x68x)>k zc3XHAyKycDn$dYs+vRTNK{;zcgxfrf`EgO++9_gUG|g4wo>BwH1gHVXP$h1da~^>K zX9WGE$TW*s%ywL|&I1sVtciJ;#e_TIZ-pVJeJv<`OOQ7)rGYh{d$Vu|5w~&vbB;J+ln!ihyMsXd*W!=?dWgfQd#ug{TkXKYz9z9rbC{F=}{7+ zhe(FI#yWnZeYuLH_dVC$GZCsCRu8(UR;56hKM^>Yo5GD7|%{Y()PA-JBXUN5UH=q8;~qpg`D^b%$=?D$m?b_@sJLYTf)J5iYaRIj4b^qiJJ zX*vV;9sK}ynJRj3Pq_Ai5~V<5{bw?!A2g>+j$~Hq%W|c`)%s=sT(PcvCN0fI$~xQb zD5m9?_=-Pb6-rMbpR*OP!=s}_35hK0qF?oa4RH=A^6Ok;M_kozw4P-MrAL1nDDFjB zO&Jk`C>5R7DXau4=s_?|W62x}^(=^j4fIkLJa|#+n@tld6o&VB-+b?TZ{ECI5$E9C z#w#;-Y+Rjr-izmE$|Lso5=X&rfn3CiL~F$PQ<50myJe|?lPrPzF&iFtSlhGMIM)Eo03yNk1*U$=pbIT#gM0^MZ_a% z!Q3Vx-Da`nMg^YWof5G%jSiJmhGKV%*#>uZ2Nz8UbBF6)*^_V>(gILLpWi=uyBkKu|5HYO0 zPQ+v7Dd9<}g?epeP&&aoWLV1SQQ;9^Bl3zW?Hp4vwOf~}NuSigi~>yn1+FIT^pdX= znJFmxWp%S(#cJAFZ1~JX2gFfCaX9F`V{Xmfq2)i+zr~jto)4Jr_nk2-v z4=mRdS9eqV_P^rkwAd-B5m&cIO7M=*i`Sk(c-KIyL~E+|#3yKnia9_9eP#Iu$sukzQ z;XK$5)^9dNxh+XSJ*5&tnHr>r(611>W;Px6t@?uXRl;JsZ_)Y4IsA+*MC|U3irfmu zoDY=+ZVnV(9qj1%IMuDNr#Rb)4DRccfZ|{er*4cJZi_R&*lYdJ@zl#`Y?u{eCatcb L;{C;zG`9Z&YW;YJ diff --git a/examples/war.nes b/examples/war.nes index daeda7e0cc086ff42959ec4c60c4c6fc96990994..bfdc2c1da50afb8bdf4c755aab7b9ce558ecd73e 100644 GIT binary patch literal 24592 zcmeHNdvp}nd7s_8+MQiJR$!T|gbZVY0AYcbM{L<_&^A$0LDvp3eh}wz_Bdf9r6T3D zX(P9)v;uBO<4(#s>7l_VvWeocxxy2$aUARb*2@x$0WDw)1PK<#$oL<5F{cOv*7p1E zJa$$fW9PrNGYHN7-rxP*@7}q0Mh|>-Wr@Qfm{-Gzw(u{$5N=C9#ROT9WQQc*8fFB4 z1!W;DyTVMA9bq{q%!CT3RCtvMZ9=O6KMpq5DmWRmNUPv#6>?eycdOtzpFE@K#lfBG zlvtJQmcTdsnoOPS6eU}Q7oD<4+TS51FQI~DDlF6#+FYheDtSq92I^AOsgOE>k^{;2 z*hips+Ttdhyu_wK;!L0cT1-pmR9Z@Jqtobg=AaV29r$<98FVHsqqFF2ItRGrbS`l3 zq!n}?olozgLAroFPZvU(zb0}kp+wqoP!UqsKuJ(YLP(OxZHE#LxINjQaKi0yGC|-r zG7u^0R2IrllX8dr3{jsZ%b~44gC;plh&tgga6=Q89N@=^gjkMMxs}z!>_)BI)W#o<0 zP1vVR7^?{A_Pwi22Aa64}uWs0COF zErxpu+^2FBrNJ)g?ry2Q8%O13qP|R)!-d4}iTZm|?#JdUM9Go=K-5=o7_N59yNUV- zz4Z|0?ZiD-n#&tr{= zU_sjAbgHkBEyOwO)&2$QM7mDx$S8K0ipn~R9=Ji(^=c=mD(lf@r>S|p%eb!JqIRKv z3+i{7`bvDf=R)w@ZF=rDHLv%)j(aYoEozVHxyRJc${FE(1L5r@%mYaDA`;AF0`bOp zAmU6k)kH%rJK0_h2IkdKU|vH{z0Tb_h=`!IFrSVA^BLM%iE=%F0JG%Serl4sb_*uU zk!3f|6}#Vu-QQ1?a(&4<-U}sJsP;jfsrw1dkviXqTQsn$SC7WuBou!L?QODRiC7 zAT<`|1gNWL~@pWU0 zf8Ef|UO3KxUJ+f#R+!;eOsrT6ir+95zcH~Qc+@jk&mJ`LeQ;uZaBOD0(#Uw_#QJj} zk%!Dg9-3G&2#ODziVsh$2p-LhA2Bk1WMX}AY-aqZk@2H9)VJ2&l!H)ve+5+oduYB_ zHp&o+)J;9o$^@GuJMmaR!mN?zv6D0(*1$_1dDO$w)Cb#Zm(-ZhwvbxM)}hOcFX+0#?PqX`MGH>BK-;;`qZMpB^{}^S0W2{mDJ*gz%ZlIAMY^@(~O z!=o4u1HipISQ&?_192~4!S{YC3OO& z$_6+uC=^Z%!*>qPQ|2W?eDPm}K3t$bPpNK-t6^7v!ic=)p$$Du$RktLW$p;w?AlntqXGM7$=q9{*8d2WQ`3Y;8U z`teM7XJQ_&h41%6iUCOOhH9WGUBG=h973ZL7uQ|NT?r?5w+r1Z0)xAh`TF6a7g^Hq z{F`)szWg3;Rn8IhJvc(>Md?g}Z2@p9KPUkaD*+{x8%28u%B{~K^;||+r3v+^47)7j zK?EM4OfpX*FygeC@)Kw|n}y|JqRcOMB(%fwe;_x9?F6nm4D%gI1NUxd#rwVkOONO!Koy!IlILLLu_MZiL=n&OVKn2+-T?@wEjB+y=4@#)>>;R2 zDFb&MyrvOkT$WC{xD$$kd&W88O96Ixn%3bN35i4XlIA7H;w6WBc@qQx)sbTW z5$<%rkl%oUW{z}AkNiGKXoE3|PG++e=0mJSe~A4%JjB>Z<1yyuK~yl)Mq~alZWc7n z{!2G&%QEwrW?H4KMzbH7Z)bjtP%G-k#{2!- z8~APhJbtyIcxk-f?i=_${(1cRIBvcH==O56NQ;sDlp?)XlaDV_zGvjSJOCoVN9ud1 znLX{bh>a+yF(W>SWWfY;Vw)z+o)FrSuNS@cC|JPSVMplZJHk^p+7W)94c!7bdAS%C z9&B=g`hvUgIHWn5Z^b|YQxvyik%y0)9Nal=?!)Q>!w2@m1NS^Jt=FRU5=*tUAfr@& zNqe4fFR<#V=ka;@cu%BVa||qehjH$>r6~7krrI0X8)Sq?V`l(I*?4kTWdq)cFiIRsXXW01j1nujwr6E;!7=o?+|&( zJpLsFocJQ&9E5Ko!+A;PaK1X~NljHRd8iXw+SiB_fkk@ck|#+tB+?o1GZ*KA* zz)3+;iW0;CB?AN3U9zP)?2pVx#HD^`TV;b1U0$I1m7xFCxP=})eHeEG_? zF}#od_r(iBJQ@y4QZO8ah6I;Twd9^fLWQfswLq*8%f&Jg+ZEyh@ryt&fc6)mjS@qr ztWXfs=`vrrufn&WNbsf8USIKjUiXxs*Z-xUR}9SYP6_&fzT4}Vbh}shQt)r=%SOOP z;D0Rw=BMw!uu=2#nqSo@&KiZ(NF{@?o)U>U<)W}MDfL}gk&CmwdBd7L+4$D2OoAuE zJy~D8`Iqh@u7F>Yfh&FuWMste!RFa9IoQR=H@CcbULqb;n>(UE{9(RfLsp04Eo@~ zMT_wNUrGysKy_nvwP4r_4KA@zYP8aoE>}Z_uCA_bbh$EgW4Iwp9tt;9S65o(=r5e1 zE2}HBeq`0+jJ&!g8m+P99|$#t0)dP?)&#UDq_?!R zbiY5E7PuVf_qz=`(9~qncMy+%dR4QDM@VmW$F;w2G2~;{E~kb%;)2KT_f$4!^y3{v zsmt&ub^0ZQ^k%n1vp3|;Rnz?*;t+;XsUau^Q{VV2noum(6pI;@AT-7rW3k2zy)+h$ z#+GL2hWc2nz9B<5MH{2hrg3yMLobc~C>9H4XdccI4?qkAEb;=k+nuFfj}bHWQM@ei z0QZ*rfxj&W%>&Mi<3ZuFkUN4;jNA>{++F zXYMv8QF(RUtG1}StzFqNf=CM;?Oni+1r!7VAp-*A2M~+XgY!1X)m<3$1olMce$U$< z(-UOT{qL^26Q{(J!9EB66!kU1ilFOde{Lv*j?$_87iL-s# zf7hRf)`u=PiR@YZS|qMZJj$zhH6PDw_%-}mK0)_!g--#^TCWCH-0F? za9eXYS+i4{r9Q_ReCjG@JjdpvZLA`b5?x9@uv7QtWUz1BO#J7-&k?_a_$B%}lgVeL zw16e~B}qb*O!9@DTEG;|-atkhNfguqO{Zo##`RT?Q(gm?%g(@-Ds|9-6h^RqYoIcbSneigqQ?r8l@(4v41G;xx z8-s`cL?4D;smdx&=#9wB{%Pcb> z9Nf?c2lp$t>}+UO4QAed!wiFEW()Z}B)`GT5K5@bWq{MDui?`a7nVMwh+LnojxD%}kF*m;H{ZTbN!W8e5sJ zh{hXCUnClDGW|Z$*v5=kSagZ3ZfC};qVbm57^UuD#%8uQ4E9RNkLF4r)}EDqKMYnf z|4`WcF}4(RwiI->7wK#zowtOatp%N8KW`Lt-YDo4`+1Xeb_hRj7IcdJY%Az&E9eyY z0i4w3oj{cCyTpBz+Q;D;$P!HQ0#U3J# zLc~@W;Q~1reZs}%ldu#nlLeK_ZYq~OO#7C##{9G&H8#rFjoLBxFkYf8-;;cvE5Fw1~D-Jh$GtM=#l`RoLo4d zO>R1cmR4`cQ>MI*FuS4gcRKn}DAfKXzcs(($!+KR~?r7#J)I-Llx&jaq zqCUj_WXiUfvaBN$YE8UEt3uEB^y?%ca=ai@$~!yN{Xjxlv@4}d*rmR0wap24fSD8U zptz4w4~ct?nicnY^&N4KsqZp9X-~N1R~DjvyDh)nmKz6)CgdHMO$Wqmy2I8eDrNn9 zj>7kBg%K_m*fAD33Wthf+-WP^=_q826z+0j+~sH#yIAOGEOa!AecWvax!X~Arzptx zZH4bU3hx#v+~dT!$I&Quaj&CsucJ}q!(A^2_Mu+>93zdRpE%&zrGvr>@{sc50li-J z(`l9^^r^fIC(&}O8)rl6xsaac5u6DF3Lh|!L1vjL`{DAtC>We2O_x^1K{p7uU&ARC z8l1wwJZ?_s)AW~kNPnDH@Dikeg#@zF6b{=sX~QKi)h1ZeXBCAfG0w1H(s5BrzDi6P znhFHO_6zoYQx|MLF||e#NApi`WzRu1Y&?sp@8ipj9=1f{Tw9JquEuf99cmn!u1(Lx z#fn+^He3gwKV6%K6if^z5!2s4O@9DWO2I_@azIa+^G^4RWkzqaNOQr{AF@bujnUgJ zlGa&yul_TO44UMMV%ujdvdomOwn%ev(ieej18fv%RX@c=KGXLKemnRW6^oCkj^t;M zP^P8@5+I2-m5zU$>uu4+Tz!w}t*Ev4#Kdg1YfW!;?X^JZ+7&baa}EHIYJtIKxcZm9 zI`sifk(RtyU`LAgLlF-M6$dq6gPi-eHr2tjom}|)r|J<@10F*Dcwxdkkh>A)ei8pQ zVLbHpjk{4GzT~F|;Ad1=;>)$=xu`akPe4&L0p1D8SmpT zY!&4evaJB8=~OjDdNq_tZWrwbBzNx}#^HkStSPjnG7cX0<^c~Vlfv!+BSAYU#}2{m z6g{CHVcLuaU&egGyOp>l`UY@~BedP7P!`Mgkd^3rn&c6bAX9#F2*}|VK+v*)8$G7h zTpmI^J=H2t4`|n+YgAL4;0rZ1CaWnHXE%BOwpQCFVMKD5>16FY&<*JUS0@9) zNt^^Xj&LHD0_yO!W`}Dtihyc~>4hG*Y^X|vm&YIgRQz2)M5hxNiW?|2OO*faUj3gmF>`FB(B0K>BYM}P;m4al#~y9!@wjt<`pjGPz2e!yv&FrE^#^ ze8l0uwXi9nf}6Us^7LYtgolH7PmQPu?4QP*jLF@SCBt+1$p-sS~P;R_& zHpKmCnJ>?C4D0jo*-(}Zp9^WB27m71{?hD^;G`leiwBbA-GiBY?v~_~!CuIBU{eY; z#OKd1353Pt$}4ygKgik7@tQBP9`E#ni2-`EpBSn@#7gqROU-tY55g2HBOd!J@Yr9O zylPNHK$>{uF+L6D>Wv&Fcg9LCTMju^E(uFia?>E1!#T?IOemGF9Qkvmva9mXDvgTU zD?TglDNmM5<(tcXTsFJxeCb=IPnF(NT37mq$ic{qk?zQSk-3rT$QR*%4et)W6#nOM zNBF_;UE${Nv~W$hEc~T>T0S8kmNW7;`4w50SINJWpO&AHJLQMvhvbgG|Hv}^aQ=TTT%`NC&p$gOb*Bb12g4Dxj~*RQ@c@~DbE2t6YV)@lf>4UJW7)D`N@%nNMmH?7%7s^ha**U!@;q! zaOHPmVL4hK9viC!eM8tcdSY1mPV6td%R|6J;QuTF&NsDxVx#rvt|#-cB1#TT<|pE{Nn@Ue+CvN1L>zs_fj!>7D2dj^Llsg1KX?r&S)*4vgykEX~z>E#c$Rg)mGjdk9U%NXS_L)=%%Z=t1Cy>1;5`%*M*B0 zE9nx#M50*;ySkc%(C@z>gcmPf5ORFj*$6p4Z2IlDXU(F2e=03Sqb(gREs||7wFl%; zN{5?n3Iy5f|^plHb7vwFisZ^^g|7g539*q{{=}yqHlwbMiqkSjOqBD~`1U|F$#SrL9^HH}L<%L?%^UPbbF3O#n=FXG1v`tdX}aLGZ<3%Y0}*3=YJ zBq>d+kgMOehiDuZ__Cx(ehGgDYuzn*pLN;lm(s_M9Xof<0j#eN^y$+MjXCtw&ptbG z!nVhp_~_7~C!e%w)cXm#T=bK46=;&?^ERD3-_+C;?>KMM`TXgVAAel1w`N^e9`h>X z3pw*2ODg7>PwE!)4%3WiXh=jKKYtOlFH|Wx`gzRjZc7ER7L%>78L%>78L%>78L%>78L%>78L%>78L%>78L%>78L%>78 UL%>78L%>78L%>7e{|5s93jiPi?EnA( diff --git a/examples/war/COMPILER_BUGS.md b/examples/war/COMPILER_BUGS.md index be072d4..4f6ab07 100644 --- a/examples/war/COMPILER_BUGS.md +++ b/examples/war/COMPILER_BUGS.md @@ -2,15 +2,30 @@ This document captures bugs and limitations discovered while building `examples/war.ne`. Each entry includes a minimal -reproduction, the symptom we observed, the root cause if known, -and a workaround we used in `examples/war/*.ne`. The intent is -to track these so they can be fixed in a future compiler pass — -once they are, the corresponding workarounds in `war/*.ne` -should be reverted to keep the example honest. +reproduction, the symptom we observed, the root cause, the +workaround originally used in `examples/war/*.ne`, and the +compiler fix that shipped (when shipped). + +## Status summary + +| # | Short name | Status | Fix commit | Regression test | +|---|---|---|---|---| +| 1 | `fun` with > 4 params silently drops the rest | **FIXED** (E0506 diagnostic) | `analyzer: reject functions with more than 4 parameters (E0506)` | `analyze_rejects_function_with_more_than_4_params`, `analyze_accepts_function_with_exactly_4_params` | +| 1b | Same-named params share VarIds across functions | **FIXED** (scope-qualified keys) | `analyzer/ir: scope function locals per function body` | `analyze_allows_same_param_name_in_two_functions` | +| 2 | Param transport slots $04-$07 clobbered by nested calls | **FIXED** (codegen prologue spill) | `codegen: spill parameters from $04-$07 into per-function RAM slots` | `codegen::ir_codegen::gen_function_prologue_spills_params_to_local_ram` | +| 3 | Function-local `var` declarations share one flat namespace | **FIXED** (scope-qualified keys) | `analyzer/ir: scope function locals per function body` | `analyze_allows_same_local_name_in_two_functions`, `analyze_allows_same_local_name_in_two_state_handlers`, `analyze_still_rejects_duplicate_local_in_same_function` | +| 4 | 8-sprites-per-scanline limit invisible to user code | Open (hardware limit; static analyzer hint could help) | — | — | +| 5 | `inline` keyword silently declined for short functions | Open | — | — | +| 6 | `wide_hi` IR map leaked between functions (u16→u8 aliasing) | **FIXED** (cleared per function) | `ir: clear wide_hi between functions to fix 16-bit op aliasing` | `ir::tests::wide_hi_does_not_leak_between_functions` | + +**Once a fix lands, revert the workaround in `examples/war/*.ne` +in the same commit** so the example keeps the game honest and +the PR diff visibly proves the fix works end-to-end. Bugs #1, +#1b, #2, #3, and #6 have had their workarounds reverted. --- -## 1. Functions with more than 4 parameters silently corrupt the 5th+ +## 1. Functions with more than 4 parameters silently corrupt the 5th+ *(FIXED)* ### Symptom @@ -94,7 +109,7 @@ Two reasonable options: --- -## 1b. Function parameters with the same name in different functions share a VarId, which collides their zero-page slot mapping +## 1b. Function parameters with the same name in different functions share a VarId, which collides their zero-page slot mapping *(FIXED)* ### Symptom @@ -171,24 +186,35 @@ function (see bug #3); we extended the same scheme to params: `pbb_card` / `dcf_card` snapshots from bug #2 stay because they also help with the bug-2 clobbering. -### Fix proposal +### Fix -Two layers to fix in: +Both the analyzer and the IR lowerer now qualify function-body +`var` / parameter declarations with the enclosing function name +(or state handler name) under an internal key +`"__local__{scope}__{name}"`. Each function's locals and +parameters therefore get **distinct** symbol-table entries and +VarIds even when the source names collide. -1. **IR lowering**: give every function its own `var_map` for - parameters and locals. The global `var_map` should only hold - top-level `var` / `const` / `enum` symbols. +Lookups inside a function body go through +`Analyzer::resolve_symbol` / `LoweringContext::scoped_key`, +which prefer the scope-qualified key over the bare one — so +a function-local `var x` correctly shadows a same-named global +(or another function's `var x`). -2. **Codegen**: even after the IR fix, the global `var_addrs` - `HashMap` should grow a per-function dimension (one map per - `IrFunction`) so two different functions can independently - assign their own VarIds to overlapping zero-page slots. +State-level locals (declared at `state Foo { var x: u8 }` +outside any handler) stay in the global namespace so every +handler in the state can read/write them across frames. -Either fix alone is probably enough; both together is robust. +See `src/analyzer/mod.rs::resolve_symbol` / `resolve_key` / +`scoped_name` and `src/ir/lowering.rs::scoped_key`. + +Together with fix #2 below, bugs #1b and #2 are completely +gone: the workaround-prefixed locals and params in `war/*.ne` +(the `dcf_`, `dwp_`, `pba_`, etc tags) are all reverted. --- -## 2. Function parameters share zero-page slots with nested calls — values clobbered across `JSR` +## 2. Function parameters share zero-page slots with nested calls — values clobbered across `JSR` *(FIXED)* ### Symptom @@ -231,24 +257,38 @@ throughout the body. See `war/render.ne::draw_card_face`, `war/render.ne::draw_flying_card`, `war/deck.ne::push_back_a`, `war/deck.ne::push_back_b`. -### Fix proposal +### Fix -1. **Spill on entry**: at the top of every function body that - makes a call, copy `$04..$07` into per-function RAM slots and - rewrite all parameter reads to load from the RAM copies. - Equivalent to what users are doing manually today. +`codegen::ir_codegen::IrCodeGen::new` now allocates every +function-local — including its parameters — into a dedicated +per-function RAM slot at `$0300+`. Parameters are still passed +via the zero-page transport slots `$04-$07` as the calling +convention, but `gen_function` now emits a 4-instruction +**prologue** at every function entry: -2. **Smarter scheduling**: only spill a parameter slot if it's - live across a call site (CFG-aware liveness pass on params). - Same effect, less RAM cost for short helpers that never read - their params after calling out. +``` +LDA $04 ; transport slot 0 +STA +LDA $05 ; transport slot 1 +STA +... etc ... +``` -Either fix would let users write straightforward function bodies -without having to remember the snapshot dance. +By the time the body runs, every parameter lives in the +function's dedicated RAM slot, so any nested call can freely +clobber `$04-$07` (passing its own arguments to _its_ callee) +without corrupting the caller's saved parameters. + +The cost is 4 LDA/STA pairs at every function entry (≈ 20 +bytes of ROM, 16 cycles). Worth it to make the calling +convention sound. + +See `codegen::ir_codegen::gen_function_prologue_spills_params_to_local_ram` +for the regression test. --- -## 3. Function-local variable names are in a flat global namespace +## 3. Function-local variable names are in a flat global namespace *(FIXED)* ### Symptom @@ -302,18 +342,23 @@ identifying its enclosing function (e.g. `dfa_card` in `dwp_px` in `draw_word_player`). This makes long files harder to read but is fully mechanical. -### Fix proposal +### Fix -Rework `register_var` to maintain a stack of scopes (one per -function body, one per nested block). Each `Statement::VarDecl` -inserts into the current scope. Lookup walks the stack from -innermost to outermost. The existing global symbol table is -unchanged for top-level globals / consts / fun names; only -function-locals shift to the scoped table. +Same as #1b: the analyzer and IR lowerer now internally +qualify function-body `var` declarations with the enclosing +scope's name, so `foo`'s `var i` and `bar`'s `var i` resolve +to `__local__foo__i` and `__local__bar__i` respectively. The +two entries coexist peacefully in the (still-flat) symbol +table. -A smaller intermediate fix: keep the flat table but qualify -each local's stored name as `::` so the global -table sees unique entries even when source names collide. +What *didn't* change: two `var i` declarations inside the +same function body still collide with E0501 (we scoped per +function body, not per nested block). That's a deliberate +trade-off — per-block scoping would require live-range +analysis to reuse RAM slots across blocks, which is a much +bigger change. The analyzer test +`analyze_still_rejects_duplicate_local_in_same_function` +pins this behaviour. --- diff --git a/examples/war/compare.ne b/examples/war/compare.ne index 319cc65..bd9f2fa 100644 --- a/examples/war/compare.ne +++ b/examples/war/compare.ne @@ -2,28 +2,24 @@ // // Cards are packed as `(rank << 4) | suit`; the extractors are // one shift or one mask each. -// -// Parameter names are unique across the entire program — see -// COMPILER_BUGS.md §1b for why same-named params in different -// functions silently corrupt each other through shared VarIds. -inline fun card_rank(crk_c: u8) -> u8 { - return crk_c >> 4 +inline fun card_rank(card: u8) -> u8 { + return card >> 4 } -inline fun card_suit(csu_c: u8) -> u8 { - return csu_c & 0x0F +inline fun card_suit(card: u8) -> u8 { + return card & 0x0F } // Compare two cards by rank. Returns: -// 1 if A wins, 2 if B wins, 0 if they tie -fun compare_cards(cmp_a: u8, cmp_b: u8) -> u8 { - var cmp_ra: u8 = card_rank(cmp_a) - var cmp_rb: u8 = card_rank(cmp_b) - if cmp_ra > cmp_rb { +// 1 if A wins, 2 if B wins, 0 if they tie. +fun compare_cards(a: u8, b: u8) -> u8 { + var ra: u8 = card_rank(a) + var rb: u8 = card_rank(b) + if ra > rb { return 1 } - if cmp_rb > cmp_ra { + if rb > ra { return 2 } return 0 diff --git a/examples/war/deal_state.ne b/examples/war/deal_state.ne index dd690d1..5e35486 100644 --- a/examples/war/deal_state.ne +++ b/examples/war/deal_state.ne @@ -47,36 +47,36 @@ state Deal { // deal_next counter controls how many of the 52 have // "landed", so the first half goes to A and the second // half to B — matching the actual split_decks() logic. - var dl_dealt_a: u8 = deal_next - var dl_dealt_b: u8 = 0 - if dl_dealt_a > HALF_DECK { - dl_dealt_b = dl_dealt_a - HALF_DECK - dl_dealt_a = HALF_DECK + var dealt_a: u8 = deal_next + var dealt_b: u8 = 0 + if dealt_a > HALF_DECK { + dealt_b = dealt_a - HALF_DECK + dealt_a = HALF_DECK } // Both decks drawn as card backs whenever they have at // least one card. Before that, skip the draw so the slot // is empty. - if dl_dealt_a > 0 { + if dealt_a > 0 { draw_card_back(DECK_A_X, DECK_Y) } - if dl_dealt_b > 0 { + if dealt_b > 0 { draw_card_back(DECK_B_X, DECK_Y) } - draw_count(COUNT_A_X, COUNT_Y, dl_dealt_a) - draw_count(COUNT_B_X, COUNT_Y, dl_dealt_b) + draw_count(COUNT_A_X, COUNT_Y, dealt_a) + draw_count(COUNT_B_X, COUNT_Y, dealt_b) // ── Flying card ────────────────────────────────── // A single face-down card bouncing between the centre // and each deck. The x position alternates based on the // low bit of deal_next (even → going to A, odd → B). if deal_next < DECK_SIZE { - var dl_fly_x: u8 = DECK_A_X + 32 + var fly_x_pos: u8 = DECK_A_X + 32 if (deal_next & 1) != 0 { - dl_fly_x = DECK_B_X - 32 + fly_x_pos = DECK_B_X - 32 } - draw_card_back(dl_fly_x, 96) + draw_card_back(fly_x_pos, 96) } // ── Transition ─────────────────────────────────── diff --git a/examples/war/deck.ne b/examples/war/deck.ne index 4af0891..7661a89 100644 --- a/examples/war/deck.ne +++ b/examples/war/deck.ne @@ -6,12 +6,6 @@ // the buffer. Everything wraps mod 52 — since 52 isn't a power // of two, we use `if idx >= 52 { idx -= 52 }` instead of `%` to // avoid the expensive software mod routine. -// -// NEScript v0.1 has a flat global symbol table for every `var` -// declaration (function-locals included), so each function's -// locals are prefixed with the function's short name to avoid -// E0501 collisions across the program. Function parameters ARE -// scoped per-function, so we can keep their names short. // ── Helpers shared by every deck ────────────────────────── // @@ -19,19 +13,11 @@ // needed inside push_back because `front` and `count` are both // ≤ 51 by construction, so the sum is at most 102 and one // subtraction is enough. -// -// Every parameter name in this file is unique across the entire -// program. NEScript v0.1's IR lowering uses a single global -// var_map for parameter names, so two functions both named -// `wrap52(v: u8)` and `another(v: u8)` end up sharing a VarId -// and the codegen routes their parameter reads to whichever -// zero-page slot the LAST function to be lowered claimed. See -// COMPILER_BUGS.md §1b for the gory details. -inline fun wrap52(w52_v: u8) -> u8 { - if w52_v >= DECK_SIZE { - return w52_v - DECK_SIZE +inline fun wrap52(v: u8) -> u8 { + if v >= DECK_SIZE { + return v - DECK_SIZE } - return w52_v + return v } // ── deck_a ──────────────────────────────────────────────── @@ -44,20 +30,15 @@ fun deck_a_empty() -> u8 { } fun draw_front_a() -> u8 { - var dfa_card: u8 = deck_a[deck_a_front] + var card: u8 = deck_a[deck_a_front] deck_a_front = wrap52(deck_a_front + 1) deck_a_count -= 1 - return dfa_card + return card } -fun push_back_a(pba_in: u8) { - // Snapshot `pba_in` into a local before calling wrap52, - // because NEScript v0.1's parameter-passing ABI uses fixed - // zero-page slots — wrap52's first param shares slot $04 - // with our `pba_in` and would silently clobber it. - var pba_card: u8 = pba_in - var pba_slot: u8 = wrap52(deck_a_front + deck_a_count) - deck_a[pba_slot] = pba_card +fun push_back_a(card: u8) { + var slot: u8 = wrap52(deck_a_front + deck_a_count) + deck_a[slot] = card deck_a_count += 1 } @@ -71,23 +52,22 @@ fun deck_b_empty() -> u8 { } fun draw_front_b() -> u8 { - var dfb_card: u8 = deck_b[deck_b_front] + var card: u8 = deck_b[deck_b_front] deck_b_front = wrap52(deck_b_front + 1) deck_b_count -= 1 - return dfb_card + return card } -fun push_back_b(pbb_in: u8) { - var pbb_card: u8 = pbb_in - var pbb_slot: u8 = wrap52(deck_b_front + deck_b_count) - deck_b[pbb_slot] = pbb_card +fun push_back_b(card: u8) { + var slot: u8 = wrap52(deck_b_front + deck_b_count) + deck_b[slot] = card deck_b_count += 1 } // ── pot ─────────────────────────────────────────────────── -fun push_back_pot(pbp_in: u8) { - pot[pot_count] = pbp_in +fun push_back_pot(card: u8) { + pot[pot_count] = card pot_count += 1 } @@ -98,19 +78,19 @@ fun clear_pot() { // Transfer every card currently in the pot into deck_a, in FIFO // order (so the face-up and face-down cards layer naturally). fun pot_to_a() { - var pta_i: u8 = 0 - while pta_i < pot_count { - push_back_a(pot[pta_i]) - pta_i += 1 + var i: u8 = 0 + while i < pot_count { + push_back_a(pot[i]) + i += 1 } pot_count = 0 } fun pot_to_b() { - var ptb_i: u8 = 0 - while ptb_i < pot_count { - push_back_b(pot[ptb_i]) - ptb_i += 1 + var i: u8 = 0 + while i < pot_count { + push_back_b(pot[i]) + i += 1 } pot_count = 0 } @@ -125,43 +105,42 @@ fun pot_to_b() { // half, and reset both queues' cursors. // // The random-swap shuffle is a bounded alternative to -// Fisher-Yates: it does N swaps between two random indices, where -// each index is rand() & 0x3F (0..63) and the swap is only done -// when both indices are < 52. 200 iterations on a 52-card deck is -// empirically well-mixed and uses only bitwise ops (no multiply, -// no divide, no W0101 warning). +// Fisher-Yates: it does 200 swaps between two random indices, +// where each index is rand() & 0x3F (0..63) and the swap is +// only done when both indices are < 52. Uses only bitwise ops +// (no multiply, no divide). fun build_master_deck() { - var bmd_r: u8 = 1 - var bmd_i: u8 = 0 - while bmd_r <= RANK_KING { - var bmd_s: u8 = 0 - while bmd_s < 4 { + var r: u8 = 1 + var i: u8 = 0 + while r <= RANK_KING { + var s: u8 = 0 + while s < 4 { // Pack rank into the high nibble, suit into the low. // rank fits in 4 bits (max 13) and suit fits in 2 // bits, so the shift-and-or is exact. - var bmd_shifted: u8 = bmd_r << 4 - deck_a[bmd_i] = bmd_shifted | bmd_s - bmd_i += 1 - bmd_s += 1 + var packed: u8 = r << 4 + deck_a[i] = packed | s + i += 1 + s += 1 } - bmd_r += 1 + r += 1 } } fun shuffle_deck_a() { - var shf_k: u8 = 0 - while shf_k < 200 { - var shf_i: u8 = rand_u8() & 0x3F - var shf_j: u8 = rand_u8() & 0x3F - if shf_i < DECK_SIZE { - if shf_j < DECK_SIZE { - var shf_tmp: u8 = deck_a[shf_i] - deck_a[shf_i] = deck_a[shf_j] - deck_a[shf_j] = shf_tmp + var k: u8 = 0 + while k < 200 { + var i: u8 = rand_u8() & 0x3F + var j: u8 = rand_u8() & 0x3F + if i < DECK_SIZE { + if j < DECK_SIZE { + var tmp: u8 = deck_a[i] + deck_a[i] = deck_a[j] + deck_a[j] = tmp } } - shf_k += 1 + k += 1 } } @@ -169,10 +148,10 @@ fun shuffle_deck_a() { // first 26 stay in deck_a, the second 26 move into deck_b. // Reset both queues' front/count cursors in the process. fun split_decks() { - var spd_i: u8 = 0 - while spd_i < HALF_DECK { - deck_b[spd_i] = deck_a[HALF_DECK + spd_i] - spd_i += 1 + var i: u8 = 0 + while i < HALF_DECK { + deck_b[i] = deck_a[HALF_DECK + i] + i += 1 } deck_a_front = 0 deck_a_count = HALF_DECK diff --git a/examples/war/play_state.ne b/examples/war/play_state.ne index 6b5f40c..2d80638 100644 --- a/examples/war/play_state.ne +++ b/examples/war/play_state.ne @@ -3,10 +3,6 @@ // `phase` cycles through the P_* constants defined in // war/constants.ne. `phase_timer` counts frames inside the // current phase and is reset to 0 whenever the phase changes. -// -// All function-local var names are prefixed with the function's -// short name (or with `pf_` for "play frame") so the global -// symbol table stays collision-free. // Set the current phase and zero the timer in one shot. Inlined // so each call site is just two stores. @@ -30,57 +26,46 @@ fun draw_table() { } // Bury helper for a war: move one card from the deck into the -// pot (face-down). Must be called with a non-empty deck. Two -// near-identical helpers (one per side) keep the locals -// uniquely-named. +// pot (face-down). Must be called with a non-empty deck. fun bury_from_a() { - var bfa_c: u8 = draw_front_a() - push_back_pot(bfa_c) + var c: u8 = draw_front_a() + push_back_pot(c) } fun bury_from_b() { - var bfb_c: u8 = draw_front_b() - push_back_pot(bfb_c) + var c: u8 = draw_front_b() + push_back_pot(c) } -// Draw the BIG WAR banner — three 16x16 metasprites at the -// centre of the screen. 12 sprites total, drawn in the centre -// row so they don't conflict with the deck stacks (rows 64-87) -// or the face-up cards (rows 128-151). -// -// All offsets stepped through locals to dodge the `x + N` -// parameter aliasing bug; see draw_word_player. +// Draw the BIG WAR banner — three 16×16 metasprite letters at +// the centre of the screen. 12 sprites total, drawn in the +// centre row so they don't conflict with the deck stacks (rows +// 64-87) or the face-up cards (rows 128-151). fun draw_big_war_banner(x: u8, y: u8) { - var bwb_y1: u8 = y + 8 - var bwb_x1: u8 = x + 8 - var bwb_x2: u8 = x + 20 - var bwb_x3: u8 = x + 28 - var bwb_x4: u8 = x + 40 - var bwb_x5: u8 = x + 48 // BIG W - draw Tileset at: (x, y) frame: TILE_BIG_W_TL - draw Tileset at: (bwb_x1, y) frame: TILE_BIG_W_TR - draw Tileset at: (x, bwb_y1) frame: TILE_BIG_W_BL - draw Tileset at: (bwb_x1, bwb_y1) frame: TILE_BIG_W_BR + draw Tileset at: (x, y) frame: TILE_BIG_W_TL + draw Tileset at: (x + 8, y) frame: TILE_BIG_W_TR + draw Tileset at: (x, y + 8) frame: TILE_BIG_W_BL + draw Tileset at: (x + 8, y + 8) frame: TILE_BIG_W_BR // BIG A - draw Tileset at: (bwb_x2, y) frame: TILE_BIG_A_TL - draw Tileset at: (bwb_x3, y) frame: TILE_BIG_A_TR - draw Tileset at: (bwb_x2, bwb_y1) frame: TILE_BIG_A_BL - draw Tileset at: (bwb_x3, bwb_y1) frame: TILE_BIG_A_BR + draw Tileset at: (x + 20, y) frame: TILE_BIG_A_TL + draw Tileset at: (x + 28, y) frame: TILE_BIG_A_TR + draw Tileset at: (x + 20, y + 8) frame: TILE_BIG_A_BL + draw Tileset at: (x + 28, y + 8) frame: TILE_BIG_A_BR // BIG R - draw Tileset at: (bwb_x4, y) frame: TILE_BIG_R_TL - draw Tileset at: (bwb_x5, y) frame: TILE_BIG_R_TR - draw Tileset at: (bwb_x4, bwb_y1) frame: TILE_BIG_R_BL - draw Tileset at: (bwb_x5, bwb_y1) frame: TILE_BIG_R_BR + draw Tileset at: (x + 40, y) frame: TILE_BIG_R_TL + draw Tileset at: (x + 48, y) frame: TILE_BIG_R_TR + draw Tileset at: (x + 40, y + 8) frame: TILE_BIG_R_BL + draw Tileset at: (x + 48, y + 8) frame: TILE_BIG_R_BR } // Begin the A-side draw animation: pull the top card off deck_a, // stash it as the face-up `card_a`, arm the fly state for the // deck → play slide, and play the click sfx. // -// fly_card / fly_face_up are stuffed directly into globals -// instead of being passed to arm_fly, because arm_fly only takes -// 4 params (the v0.1 ABI limit) and silently drops anything past -// the fourth. +// fly_card / fly_face_up are written directly to globals +// instead of being passed to arm_fly, because arm_fly already +// takes 4 parameters and the v0.1 ABI caps function signatures +// at 4 parameters (E0506). fun begin_draw_a() { if deck_a_count > 0 { card_a = draw_front_a() @@ -189,16 +174,16 @@ state Playing { // Both cards go into the pot regardless of outcome. push_back_pot(card_a) push_back_pot(card_b) - pf_result = compare_cards(card_a, card_b) - if pf_result == 1 { + var result: u8 = compare_cards(card_a, card_b) + if result == 1 { play CheerA set_phase(P_WIN_A) } - if pf_result == 2 { + if result == 2 { play CheerB set_phase(P_WIN_B) } - if pf_result == 0 { + if result == 0 { // It's a tie — but only enter the war flow if both // sides actually have cards left to bury. If a // player ran out of cards on this very tie, the diff --git a/examples/war/render.ne b/examples/war/render.ne index 26c3633..6729a3f 100644 --- a/examples/war/render.ne +++ b/examples/war/render.ne @@ -6,10 +6,6 @@ // a 16×24 card face burns 6 sprite slots, a single-character // letter burns 1, and so on. The caller is responsible for // sprite budgeting — see PLAN.md §3. -// -// Function-local var names are prefixed with the function's -// short name to avoid the global symbol-table collisions that -// E0501 would otherwise complain about. // ── Card face ────────────────────────────────────────────── // @@ -22,69 +18,46 @@ // // Every tile in the card has a fully-opaque white background // (palette index 2) so the felt green behind the card does not -// bleed through — see `assets.ne` for the art itself. The big -// centre pip is a single 16×16 shape split across the bottom -// two rows (4 tiles total); `rank` sits in the top-left corner -// and `small_suit` in the top-right. -fun draw_card_face(dcf_in_x: u8, dcf_in_y: u8, dcf_in_card: u8) { - // Snapshot every parameter into a fresh local *before* any - // nested function call. NEScript v0.1 passes parameters via - // fixed zero-page slots ($04, $05, $06), and any inner call - // overwrites those slots with its own parameter values — - // including the inline `card_rank` / `card_suit` calls below, - // which would otherwise leave `x` and `y` corrupted by the - // time the draw lines run. - var dcf_x: u8 = dcf_in_x - var dcf_y: u8 = dcf_in_y - var dcf_card: u8 = dcf_in_card - var dcf_rank: u8 = card_rank(dcf_card) - var dcf_suit: u8 = card_suit(dcf_card) - var dcf_rank_tile: u8 = TILE_RANK_BASE + dcf_rank - 1 - var dcf_small_tile: u8 = TILE_SUIT_SMALL_BASE + dcf_suit - var dcf_pip_tl: u8 = TILE_PIP_TL_BASE + dcf_suit - var dcf_pip_tr: u8 = TILE_PIP_TR_BASE + dcf_suit - var dcf_pip_bl: u8 = TILE_PIP_BL_BASE + dcf_suit - var dcf_pip_br: u8 = TILE_PIP_BR_BASE + dcf_suit - var dcf_x1: u8 = dcf_x + 8 - var dcf_y1: u8 = dcf_y + 8 - var dcf_y2: u8 = dcf_y + 16 +// bleed through — see `assets.ne` for the art itself. +fun draw_card_face(x: u8, y: u8, card: u8) { + var rank: u8 = card_rank(card) + var suit: u8 = card_suit(card) + var rank_tile: u8 = TILE_RANK_BASE + rank - 1 + var small_tile: u8 = TILE_SUIT_SMALL_BASE + suit + var pip_tl: u8 = TILE_PIP_TL_BASE + suit + var pip_tr: u8 = TILE_PIP_TR_BASE + suit + var pip_bl: u8 = TILE_PIP_BL_BASE + suit + var pip_br: u8 = TILE_PIP_BR_BASE + suit // Row 0 — rank corner + small suit - draw Tileset at: (dcf_x, dcf_y) frame: dcf_rank_tile - draw Tileset at: (dcf_x1, dcf_y) frame: dcf_small_tile + draw Tileset at: (x, y) frame: rank_tile + draw Tileset at: (x + 8, y) frame: small_tile // Row 1 — top half of the 16×16 big pip - draw Tileset at: (dcf_x, dcf_y1) frame: dcf_pip_tl - draw Tileset at: (dcf_x1, dcf_y1) frame: dcf_pip_tr + draw Tileset at: (x, y + 8) frame: pip_tl + draw Tileset at: (x + 8, y + 8) frame: pip_tr // Row 2 — bottom half of the 16×16 big pip - draw Tileset at: (dcf_x, dcf_y2) frame: dcf_pip_bl - draw Tileset at: (dcf_x1, dcf_y2) frame: dcf_pip_br + draw Tileset at: (x, y + 16) frame: pip_bl + draw Tileset at: (x + 8, y + 16) frame: pip_br } -// Draw the card-back lattice at (x, y). 6 sprites again. Rows -// 0 and 2 reuse the same top/bottom back tiles; row 1 uses the -// bottom row of the lattice as a filler so the pattern stays -// continuous. +// Draw the card-back checkerboard at (x, y). 6 sprites. The +// back tiles tile seamlessly so every cell of the 16×24 card +// body carries a 2-pixel square of the same black/white grid. fun draw_card_back(x: u8, y: u8) { - var dcb_x1: u8 = x + 8 - var dcb_y1: u8 = y + 8 - var dcb_y2: u8 = y + 16 - draw Tileset at: (x, y) frame: TILE_BACK_TL - draw Tileset at: (dcb_x1, y) frame: TILE_BACK_TR - draw Tileset at: (x, dcb_y1) frame: TILE_BACK_BL - draw Tileset at: (dcb_x1, dcb_y1) frame: TILE_BACK_BR - draw Tileset at: (x, dcb_y2) frame: TILE_BACK_TL - draw Tileset at: (dcb_x1, dcb_y2) frame: TILE_BACK_TR + draw Tileset at: (x, y) frame: TILE_BACK_TL + draw Tileset at: (x + 8, y) frame: TILE_BACK_TR + draw Tileset at: (x, y + 8) frame: TILE_BACK_BL + draw Tileset at: (x + 8, y + 8) frame: TILE_BACK_BR + draw Tileset at: (x, y + 16) frame: TILE_BACK_TL + draw Tileset at: (x + 8, y + 16) frame: TILE_BACK_TR } // Dispatch between front and back based on the fly_face_up flag -// stamped by the phase handlers. Snapshots params first because -// the inner call clobbers $04/$05. +// stamped by the phase handlers. fun draw_flying_card(x: u8, y: u8) { - var dfc_x: u8 = x - var dfc_y: u8 = y if fly_face_up == 0 { - draw_card_back(dfc_x, dfc_y) + draw_card_back(x, y) } else { - draw_card_face(dfc_x, dfc_y, fly_card) + draw_card_face(x, y, fly_card) } } @@ -95,91 +68,62 @@ fun draw_digit(x: u8, y: u8, d: u8) { draw Tileset at: (x, y) frame: TILE_DIGIT_BASE + d } -// Draw a two-digit decimal count (0..99) at (x, y). Leading zero -// is preserved so the HUD always renders two glyphs wide, which -// keeps the layout stable as the counts change. +// Draw a two-digit decimal count (0..99) at (x, y). Leading +// zero is preserved so the HUD always renders two glyphs wide, +// which keeps the layout stable as the counts change. fun draw_count(x: u8, y: u8, v: u8) { - var dct_tens: u8 = 0 - var dct_n: u8 = v + var tens: u8 = 0 + var n: u8 = v // Divide by 10 without the software divide: repeatedly // subtract 10 until n < 10. For v ≤ 52 this loops at most // 5 times, cheaper than calling `/` and `%`. - while dct_n >= 10 { - dct_n -= 10 - dct_tens += 1 + while n >= 10 { + n -= 10 + tens += 1 } - draw_digit(x, y, dct_tens) - draw_digit(x + 8, y, dct_n) + draw_digit(x, y, tens) + draw_digit(x + 8, y, n) } // Draw a single letter 'A'..'Z' using the sprite font. `ch` is -// the letter index (0 = A, 25 = Z). Deliberately NOT marked -// `inline`: when this was inlined the resulting code put each -// inlined `draw` at double the intended X step (the inliner -// appears to re-evaluate the (x + N) parameter expression in a -// way that compounds across consecutive draws). Keeping it as -// a real function call gives every draw_letter call its own -// argument-evaluation context and the spacing comes out right. +// the letter index (0 = A, 25 = Z). fun draw_letter(x: u8, y: u8, ch: u8) { draw Tileset at: (x, y) frame: TILE_LETTER_BASE + ch } // Short helper for drawing the word "PLAYER" at (x, y). 6 letters. // Used by the HUD and the victory banner. -// -// We accumulate the X position in a local instead of passing -// `x + N` as a call argument: the latter pattern miscompiles in -// NEScript v0.1 (consecutive `x + N` arguments to the same -// function appear to alias the parameter slot, leaving the second -// onward call site reading a stale offset). Stepping a local -// avoids the issue entirely. fun draw_word_player(x: u8, y: u8) { - var dwp_px: u8 = x - draw_letter(dwp_px, y, 15) // P - dwp_px += 8 - draw_letter(dwp_px, y, 11) // L - dwp_px += 8 - draw_letter(dwp_px, y, 0) // A - dwp_px += 8 - draw_letter(dwp_px, y, 24) // Y - dwp_px += 8 - draw_letter(dwp_px, y, 4) // E - dwp_px += 8 - draw_letter(dwp_px, y, 17) // R + draw_letter(x, y, 15) // P + draw_letter(x + 8, y, 11) // L + draw_letter(x + 16, y, 0) // A + draw_letter(x + 24, y, 24) // Y + draw_letter(x + 32, y, 4) // E + draw_letter(x + 40, y, 17) // R } // "WINS" — used on the victory screen. fun draw_word_wins(x: u8, y: u8) { - var dww_px: u8 = x - draw_letter(dww_px, y, 22) // W - dww_px += 8 - draw_letter(dww_px, y, 8) // I - dww_px += 8 - draw_letter(dww_px, y, 13) // N - dww_px += 8 - draw_letter(dww_px, y, 18) // S + draw_letter(x, y, 22) // W + draw_letter(x + 8, y, 8) // I + draw_letter(x + 16, y, 13) // N + draw_letter(x + 24, y, 18) // S } // "PRESS" — used on the title screen. fun draw_word_press(x: u8, y: u8) { - var dwr_px: u8 = x - draw_letter(dwr_px, y, 15) // P - dwr_px += 8 - draw_letter(dwr_px, y, 17) // R - dwr_px += 8 - draw_letter(dwr_px, y, 4) // E - dwr_px += 8 - draw_letter(dwr_px, y, 18) // S - dwr_px += 8 - draw_letter(dwr_px, y, 18) // S + draw_letter(x, y, 15) // P + draw_letter(x + 8, y, 17) // R + draw_letter(x + 16, y, 4) // E + draw_letter(x + 24, y, 18) // S + draw_letter(x + 32, y, 18) // S } // ── Fly animation driver ────────────────────────────────── // -// We avoid the software multiply (and the W0101 "expensive -// multiply" warning) by stepping the fly position by a fixed -// constant each frame instead of computing `start + dx * t / -// FRAMES`. The constant FLY_STEP is chosen so that +// Avoiding a software multiply: instead of computing +// `start + dx * t / FRAMES` we step the fly position by a +// fixed constant each frame. FLY_STEP is chosen so that // FRAMES_FLY * FLY_STEP equals the deck-to-play distance on // both axes: // @@ -192,8 +136,8 @@ const FLY_STEP: u8 = 4 // Step the global (fly_x, fly_y) by FLY_STEP in the directions // stored in fly_dx_sign / fly_dy_sign. Sign 0 = positive (move -// right / down), 1 = negative (move left / up). Called once per -// frame inside the relevant fly-phase handler. +// right / down), 1 = negative (move left / up). Called once +// per frame inside the relevant fly-phase handler. fun step_fly_pos() { if fly_dx_sign == 0 { fly_x += FLY_STEP @@ -208,11 +152,9 @@ fun step_fly_pos() { } // Initialise the fly state for a card slide from (sx, sy) using -// the given direction signs. NEScript v0.1 only allocates four -// zero-page slots ($04-$07) for function parameters, so callers -// must stash `fly_card` and `fly_face_up` directly into the -// globals before invoking arm_fly — passing them as the 5th and -// 6th params would silently drop the values on the floor. +// the given direction signs, card byte, and face-up flag. The +// caller is responsible for picking signs so the end position +// lands where it should after FRAMES_FLY frames. fun arm_fly(sx: u8, sy: u8, dxsign: u8, dysign: u8) { fly_x = sx fly_y = sy diff --git a/examples/war/state.ne b/examples/war/state.ne index 2428e12..ea9055c 100644 --- a/examples/war/state.ne +++ b/examples/war/state.ne @@ -40,7 +40,6 @@ var a_is_cpu: u8 = 1 // bool-ish var b_is_cpu: u8 = 1 var phase: u8 = 0 // one of the P_* constants var phase_timer: u8 = 0 // counts up during each phase -var pf_result: u8 = 0 // P_RESOLVE comparison result (1=A, 2=B, 0=tie) // ── Animation state ─────────────────────────────────────── // Shared by every card-fly phase. We step (fly_x, fly_y) by a diff --git a/examples/war/victory_state.ne b/examples/war/victory_state.ne index e8cbc6b..4e7d47b 100644 --- a/examples/war/victory_state.ne +++ b/examples/war/victory_state.ne @@ -34,17 +34,21 @@ state Victory { // deck is empty (shouldn't happen — we only arrive // here because the *loser* is empty) fall back to a // card back so the layout is stable. + // Pull the winner's top card into a single local. Only + // one `var top` declaration per on-frame body — NEScript + // scopes locals per function body, not per if-block. + var top: u8 = 0 if winner == 0 { if deck_a_count > 0 { - var vic_top_a: u8 = deck_a[deck_a_front] - draw_card_face(120, 128, vic_top_a) + top = deck_a[deck_a_front] + draw_card_face(120, 128, top) } else { draw_card_back(120, 128) } } else { if deck_b_count > 0 { - var vic_top_b: u8 = deck_b[deck_b_front] - draw_card_face(120, 128, vic_top_b) + top = deck_b[deck_b_front] + draw_card_face(120, 128, top) } else { draw_card_back(120, 128) } diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index f33717e..3fb54d7 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -119,6 +119,7 @@ pub fn analyze(program: &Program) -> AnalysisResult { function_return_types: HashMap::new(), current_return_type: None, in_function_body: false, + current_scope_prefix: None, struct_layouts: HashMap::new(), }; analyzer.analyze_program(program); @@ -180,6 +181,24 @@ struct Analyzer { /// distinguish "void function" from "state handler" when checking /// `return value` statements. in_function_body: bool, + /// Current local scope prefix used to qualify function-body + /// `var` declarations. `None` at the top level and during + /// state-level declaration registration; set to `Some("foo")` + /// while analyzing the body of `fun foo`, and to + /// `Some("Title::frame")` (etc.) while analyzing a state + /// handler body. When it is `Some(prefix)`, `register_var` + /// stores the declaration under a qualified key + /// `"__local__{prefix}__{name}"`, and `resolve_*` helpers + /// below fall back through the qualified key first and the + /// bare key second so identifier reads inside a body resolve + /// to the locally-scoped entry when one exists. + /// + /// This is how functions and handlers get their own local + /// namespaces without requiring a full nested-scope stack. + /// Top-level globals, consts, state-level vars, function + /// names, and enum variants still live at the bare name in + /// `self.symbols`. + current_scope_prefix: Option, /// Struct name to layout. Each field has an offset in bytes from /// the base address of the struct. struct_layouts: HashMap, @@ -524,16 +543,27 @@ impl Analyzer { )); } - // Type-check all state bodies + // Type-check all state bodies. Each handler body is its + // own local scope: a `var i` inside `Title::on frame` + // does not collide with a `var i` inside `Playing::on + // frame`. State-level `var`s (declared at `state Foo { var x + // }`) stay in the global scope so every handler in the + // state can read and write them across frames. for state in &program.states { if let Some(block) = &state.on_enter { + self.current_scope_prefix = Some(format!("{}__enter", state.name)); self.check_block(block, &state_names); + self.current_scope_prefix = None; } if let Some(block) = &state.on_exit { + self.current_scope_prefix = Some(format!("{}__exit", state.name)); self.check_block(block, &state_names); + self.current_scope_prefix = None; } if let Some(block) = &state.on_frame { + self.current_scope_prefix = Some(format!("{}__frame", state.name)); self.check_block(block, &state_names); + self.current_scope_prefix = None; } // `on scanline(N)` is only valid with mappers that have a // scanline-counting IRQ source (currently only MMC3). @@ -544,43 +574,64 @@ impl Analyzer { state.span, )); } - for (_, block) in &state.on_scanline { + for (line, block) in &state.on_scanline { + self.current_scope_prefix = Some(format!("{}__scanline_{}", state.name, line)); self.check_block(block, &state_names); + self.current_scope_prefix = None; } } - // Type-check function bodies. Parameters are registered as - // symbols for the duration of the body check so that identifier - // references (and the W0103 used-variable tracker) can resolve - // them. They are unregistered afterwards to avoid leaking into - // the global scope. Parameters are also pre-marked as "used" so - // we do not emit W0103 for unused function arguments (which are - // a common and deliberate pattern). + // Type-check function bodies. Each function body gets its + // own local scope by setting `current_scope_prefix` to + // the function's name. Parameters are registered as + // function-local symbols under the prefixed key so two + // different functions can both declare a parameter named + // `x` without the analyzer's duplicate-declaration check + // firing. Each parameter also gets its own dedicated RAM + // slot allocated via `allocate_ram` — the codegen emits a + // prologue that copies the transport slots `$04-$07` + // into these RAM slots at function entry, so the param + // values survive any nested calls the function body + // makes. Parameters are pre-marked as "used" so we do + // not emit W0103 for unused function arguments. for fun in &program.functions { - let mut added_params = Vec::new(); + self.current_scope_prefix = Some(fun.name.clone()); for param in &fun.params { - if !self.symbols.contains_key(¶m.name) { + let key = self.scoped_name(¶m.name); + let size = match param.param_type { + NesType::U8 | NesType::I8 | NesType::Bool => 1, + NesType::U16 => 2, + // Struct/array parameters are not supported + // in v0.1; the parser already rejects them, + // so defaulting to 1 byte here is just a + // fallback to keep the analyzer from + // crashing on malformed ASTs. + _ => 1, + }; + if let Some(address) = self.allocate_ram(size, fun.span) { self.symbols.insert( - param.name.clone(), + key.clone(), Symbol { - name: param.name.clone(), + name: key.clone(), sym_type: param.param_type.clone(), is_const: false, span: fun.span, }, ); - added_params.push(param.name.clone()); + self.var_allocations.push(VarAllocation { + name: key.clone(), + address, + size, + }); + self.used_vars.insert(key); } - self.mark_var_used(¶m.name); } self.current_return_type.clone_from(&fun.return_type); self.in_function_body = true; self.check_block(&fun.body, &state_names); self.current_return_type = None; self.in_function_body = false; - for name in &added_params { - self.symbols.remove(name); - } + self.current_scope_prefix = None; } // Build call graph @@ -629,18 +680,68 @@ impl Analyzer { self.check_unreachable_states(program); } + /// Qualify `name` under the current scope prefix. If no prefix + /// is active (top-level analysis, state-level declaration) the + /// name is returned unchanged. Inside a function body the + /// result is `"__local__{prefix}__{name}"` — the same key the + /// IR lowerer and codegen use when walking a function's locals. + fn scoped_name(&self, name: &str) -> String { + match &self.current_scope_prefix { + Some(prefix) => format!("__local__{prefix}__{name}"), + None => name.to_string(), + } + } + + /// Resolve a user-written identifier to the matching symbol + /// table key. Tries the current scope's qualified key first + /// (so function-local vars shadow same-named globals) and + /// falls back to the bare key for globals, consts, enum + /// variants, state-locals, and function names. + fn resolve_key(&self, name: &str) -> String { + if let Some(prefix) = &self.current_scope_prefix { + let qualified = format!("__local__{prefix}__{name}"); + if self.symbols.contains_key(&qualified) { + return qualified; + } + } + name.to_string() + } + + /// Look up a user-written identifier in the symbol table, + /// preferring the current scope's qualified entry (if any) + /// and falling back to the bare global entry. + fn resolve_symbol(&self, name: &str) -> Option<&Symbol> { + if let Some(prefix) = &self.current_scope_prefix { + let qualified = format!("__local__{prefix}__{name}"); + if let Some(sym) = self.symbols.get(&qualified) { + return Some(sym); + } + } + self.symbols.get(name) + } + + /// True if a symbol exists for `name` in the current scope or + /// in the global scope. + fn symbol_exists(&self, name: &str) -> bool { + self.resolve_symbol(name).is_some() + } + /// Mark a variable name as having been read somewhere in the program. - /// Also bumps the W0107 access counter. + /// Also bumps the W0107 access counter. Resolves the name through + /// the current scope so the right (qualified-or-bare) key is + /// recorded. fn mark_var_used(&mut self, name: &str) { - self.used_vars.insert(name.to_string()); - self.bump_var_access(name); + let key = self.resolve_key(name); + self.used_vars.insert(key.clone()); + *self.var_access_counts.entry(key).or_insert(0) += 1; } /// Increment the observed access count for `name`. Called for /// both reads (via [`Analyzer::mark_var_used`]) and writes (at /// `Statement::Assign` handling). Used for W0107. fn bump_var_access(&mut self, name: &str) { - *self.var_access_counts.entry(name.to_string()).or_insert(0) += 1; + let key = self.resolve_key(name); + *self.var_access_counts.entry(key).or_insert(0) += 1; } /// Emit W0107 if `var` is declared `fast` but sees fewer than @@ -712,7 +813,7 @@ impl Analyzer { fn walk_expr_reads(&mut self, expr: &Expr) { match expr { Expr::Ident(name, span) => { - if self.symbols.contains_key(name) { + if self.symbol_exists(name) { self.mark_var_used(name); } else { self.emit_undefined_var(name, *span); @@ -720,7 +821,7 @@ impl Analyzer { } Expr::ArrayIndex(name, idx, span) => { // Array base is a read; index may contain more reads. - if self.symbols.contains_key(name) { + if self.symbol_exists(name) { self.mark_var_used(name); } else { self.emit_undefined_var(name, *span); @@ -728,14 +829,15 @@ impl Analyzer { self.walk_expr_reads(idx); } Expr::FieldAccess(name, field, span) => { - // Resolve the struct variable and verify the field - // exists. Mark the synthetic `name.field` variable as - // used so W0103 doesn't fire. - let full_name = format!("{name}.{field}"); + // Resolve the struct variable through the scope + // stack and verify the field exists. Mark both the + // base and the synthetic `name.field` entry used. + let base_key = self.resolve_key(name); + let full_name = format!("{base_key}.{field}"); if self.symbols.contains_key(&full_name) { self.mark_var_used(name); - self.mark_var_used(&full_name); - } else if !self.symbols.contains_key(name) { + self.used_vars.insert(full_name); + } else if !self.symbols.contains_key(&base_key) { self.emit_undefined_var(name, *span); } else { self.diagnostics.push(Diagnostic::error( @@ -1134,7 +1236,19 @@ impl Analyzer { } fn register_var(&mut self, var: &VarDecl) { - if self.symbols.contains_key(&var.name) { + // Scope-qualified storage key. At the top level this is + // the bare `var.name`; inside a function or handler body + // it is `"__local__{prefix}__{name}"` so two different + // functions can each declare a local `var i` without + // colliding on the flat symbol table. + let key = self.scoped_name(&var.name); + + // Duplicate check runs against the qualified key so + // shadowing a global with a function-local var is fine + // (the local key is distinct). Two locals with the same + // name inside the same scope still collide and report + // E0501 correctly. + if self.symbols.contains_key(&key) { self.diagnostics.push(Diagnostic::error( ErrorCode::E0501, format!("duplicate declaration of '{}'", var.name), @@ -1194,9 +1308,9 @@ impl Analyzer { // symbol so that later references don't cascade into E0502, // but don't record a var_allocations entry. self.symbols.insert( - var.name.clone(), + key.clone(), Symbol { - name: var.name.clone(), + name: key, sym_type: var.var_type.clone(), is_const: false, span: var.span, @@ -1215,15 +1329,21 @@ impl Analyzer { // `p.pos.y` leaves. Array fields produce a single // synthetic with the array type so the existing // `Expr::ArrayIndex` lowering picks them up. + // + // Struct fields use the qualified key as the base so + // function-local struct instances don't collide with + // same-named globals. `flatten_struct_fields` builds + // `"{key}.{field}"` paths, which inherits the scope + // prefix automatically. if let NesType::Struct(sname) = &var.var_type { let layout = self.struct_layouts[sname].clone(); - self.flatten_struct_fields(&var.name, address, &layout, var.span); + self.flatten_struct_fields(&key, address, &layout, var.span); // Also register the struct variable itself (as a symbol // only — it doesn't have a single VarAllocation entry). self.symbols.insert( - var.name.clone(), + key.clone(), Symbol { - name: var.name.clone(), + name: key, sym_type: var.var_type.clone(), is_const: false, span: var.span, @@ -1233,9 +1353,9 @@ impl Analyzer { } self.symbols.insert( - var.name.clone(), + key.clone(), Symbol { - name: var.name.clone(), + name: key.clone(), sym_type: var.var_type.clone(), is_const: false, span: var.span, @@ -1243,7 +1363,7 @@ impl Analyzer { ); self.var_allocations.push(VarAllocation { - name: var.name.clone(), + name: key, address, size, }); @@ -1441,10 +1561,12 @@ impl Analyzer { } } Statement::Assign(lvalue, _, expr, span) => { - // Check if trying to assign to a constant + // Check if trying to assign to a constant. Lookups + // go through `resolve_symbol` so a function-local + // shadowing a global const/var is handled correctly. match lvalue { LValue::Var(name) => { - if let Some(sym) = self.symbols.get(name) { + if let Some(sym) = self.resolve_symbol(name) { if sym.is_const { self.diagnostics.push(Diagnostic::error( ErrorCode::E0203, @@ -1467,7 +1589,7 @@ impl Analyzer { } } LValue::ArrayIndex(name, idx) => { - if let Some(sym) = self.symbols.get(name) { + if let Some(sym) = self.resolve_symbol(name) { if sym.is_const { self.diagnostics.push(Diagnostic::error( ErrorCode::E0203, @@ -1484,13 +1606,20 @@ impl Analyzer { self.walk_expr_reads(idx); } LValue::Field(name, field) => { - let full_name = format!("{name}.{field}"); + // Struct instances are stored under the + // scope-qualified key (see register_var); + // the per-field synthetic keys inherit that + // prefix via `flatten_struct_fields`. Build + // the field path off whichever key actually + // exists — scoped first, bare second. + let base_key = self.resolve_key(name); + let full_name = format!("{base_key}.{field}"); if self.symbols.contains_key(&full_name) { // Assigning to a field is a mutation; don't // mark the struct variable as "read" just // because we wrote to one of its fields. - self.mark_var_used(&full_name); - } else if self.symbols.contains_key(name) { + self.used_vars.insert(full_name); + } else if self.symbols.contains_key(&base_key) { self.diagnostics.push(Diagnostic::error( ErrorCode::E0201, format!("'{name}' has no field '{field}'"), @@ -1558,11 +1687,16 @@ impl Analyzer { self.walk_expr_reads(end); self.check_expr_type(start, &NesType::U8); self.check_expr_type(end, &NesType::U8); - let was_shadowed = self.symbols.remove(var); + // Register the loop variable under the current + // scope's qualified key so two `for i in 0..n` + // loops in different functions get their own + // per-function RAM slot. + let loop_key = self.scoped_name(var); + let was_shadowed = self.symbols.remove(&loop_key); self.symbols.insert( - var.clone(), + loop_key.clone(), Symbol { - name: var.clone(), + name: loop_key.clone(), sym_type: NesType::U8, is_const: false, span: *span, @@ -1573,19 +1707,19 @@ impl Analyzer { // other u8 local. let loop_var_addr = self.allocate_ram(1, *span).unwrap_or(0x10); self.var_allocations.push(VarAllocation { - name: var.clone(), + name: loop_key.clone(), address: loop_var_addr, size: 1, }); // Loop variable is always "used" in the header. - self.mark_var_used(var); + self.used_vars.insert(loop_key.clone()); let was_in_loop = self.in_loop; self.in_loop = true; self.check_block(body, state_names); self.in_loop = was_in_loop; - self.symbols.remove(var); + self.symbols.remove(&loop_key); if let Some(old) = was_shadowed { - self.symbols.insert(var.clone(), old); + self.symbols.insert(loop_key, old); } } Statement::Loop(body, span) => { @@ -1780,15 +1914,17 @@ impl Analyzer { fn lvalue_type(&self, lvalue: &LValue, _span: Span) -> Option { match lvalue { - LValue::Var(name) => self.symbols.get(name).map(|s| s.sym_type.clone()), + LValue::Var(name) => self.resolve_symbol(name).map(|s| s.sym_type.clone()), LValue::ArrayIndex(name, _) => { - self.symbols.get(name).and_then(|sym| match &sym.sym_type { - NesType::Array(elem, _) => Some(elem.as_ref().clone()), - _ => None, - }) + self.resolve_symbol(name) + .and_then(|sym| match &sym.sym_type { + NesType::Array(elem, _) => Some(elem.as_ref().clone()), + _ => None, + }) } LValue::Field(name, field) => { - let full_name = format!("{name}.{field}"); + let base_key = self.resolve_key(name); + let full_name = format!("{base_key}.{field}"); self.symbols.get(&full_name).map(|s| s.sym_type.clone()) } } @@ -1873,7 +2009,7 @@ impl Analyzer { } } Expr::BoolLiteral(_, _) => Some(NesType::Bool), - Expr::Ident(name, _) => self.symbols.get(name).map(|s| s.sym_type.clone()), + Expr::Ident(name, _) => self.resolve_symbol(name).map(|s| s.sym_type.clone()), Expr::ButtonRead(_, _, _) => Some(NesType::Bool), Expr::BinaryOp(_, op, _, _) => match op { BinOp::Eq @@ -1890,13 +2026,14 @@ impl Analyzer { Expr::UnaryOp(_, _, _) => Some(NesType::U8), Expr::Call(_, _, _) => Some(NesType::U8), // Simplified for M1 Expr::ArrayIndex(name, _, _) => { - self.symbols.get(name).and_then(|s| match &s.sym_type { + self.resolve_symbol(name).and_then(|s| match &s.sym_type { NesType::Array(elem, _) => Some(elem.as_ref().clone()), _ => None, }) } Expr::FieldAccess(name, field, _) => { - let full_name = format!("{name}.{field}"); + let base_key = self.resolve_key(name); + let full_name = format!("{base_key}.{field}"); self.symbols.get(&full_name).map(|s| s.sym_type.clone()) } Expr::ArrayLiteral(_, _) => Some(NesType::U8), // element type inferred from context diff --git a/src/analyzer/tests.rs b/src/analyzer/tests.rs index 103e72b..c87c297 100644 --- a/src/analyzer/tests.rs +++ b/src/analyzer/tests.rs @@ -2100,3 +2100,103 @@ fn analyze_accepts_function_with_exactly_4_params() { "#, ); } + +#[test] +fn analyze_allows_same_local_name_in_two_functions() { + // Regression test for COMPILER_BUGS.md §3: function-body + // `var` declarations used to live in a flat global namespace, + // so two functions both declaring `var i` collided on E0501. + // They should now coexist in their own per-function scopes. + analyze_ok( + r#" + game "T" { mapper: NROM } + fun foo() -> u8 { + var i: u8 = 0 + while i < 5 { i += 1 } + return i + } + fun bar() -> u8 { + var i: u8 = 0 + while i < 10 { i += 1 } + return i + } + var total: u8 = 0 + on frame { + total = foo() + bar() + wait_frame + } + start Main + "#, + ); +} + +#[test] +fn analyze_allows_same_param_name_in_two_functions() { + // Regression test for COMPILER_BUGS.md §1b: parameters + // across different functions used to share VarIds because + // the IR lowerer's `var_map` was global. Both declaration + // (analyzer) and lowering (IR) should now give each + // function's parameters their own independent entries. + analyze_ok( + r#" + game "T" { mapper: NROM } + fun shift_left(x: u8) -> u8 { return x << 1 } + fun shift_right(x: u8) -> u8 { return x >> 1 } + var n: u8 = 0 + on frame { + n = shift_left(5) + shift_right(20) + wait_frame + } + start Main + "#, + ); +} + +#[test] +fn analyze_allows_same_local_name_in_two_state_handlers() { + // Each state handler gets its own local scope, so both + // `Title::on frame` and `Playing::on frame` can declare + // `var i` independently. + analyze_ok( + r#" + game "T" { mapper: NROM } + state Title { + on frame { + var i: u8 = 0 + while i < 3 { i += 1 } + if button.start { transition Playing } + } + } + state Playing { + on frame { + var i: u8 = 0 + while i < 7 { i += 1 } + } + } + start Title + "#, + ); +} + +#[test] +fn analyze_still_rejects_duplicate_local_in_same_function() { + // Two `var i` declarations inside the SAME function body + // should still trip E0501 — we scoped locals per function, + // not per statement. + let errors = analyze_errors( + r#" + game "T" { mapper: NROM } + fun foo() -> u8 { + var i: u8 = 0 + var i: u8 = 1 + return i + } + on frame { var r: u8 = foo() wait_frame } + start Main + "#, + ); + assert!( + errors.contains(&ErrorCode::E0501), + "expected E0501 for duplicate `var i` in same function, got: {errors:?}" + ); +} diff --git a/src/codegen/ir_codegen.rs b/src/codegen/ir_codegen.rs index 51d4950..1332f49 100644 --- a/src/codegen/ir_codegen.rs +++ b/src/codegen/ir_codegen.rs @@ -210,13 +210,27 @@ impl<'a> IrCodeGen<'a> { var_sizes.insert(global.var_id, alloc.size); } } - // Map each function's parameter VarIds to the zero-page - // parameter-passing slots $04-$07 (up to 4 params). Map the - // rest of each function's locals into main RAM starting at - // `$0300` (after the OAM buffer). Locals don't overlap with - // globals (which were placed by the analyzer) and are - // disjoint across functions so nested calls don't corrupt - // each other. + // Map every function-local — parameters AND body-declared + // vars — into a dedicated RAM slot at `$0300+`. Parameters + // are still passed via the zero-page transport slots + // `$04-$07` as the calling convention, but `gen_function` + // emits a prologue at function entry that copies those + // transport slots into these per-function RAM slots. That + // way, when a function makes a nested call, the nested + // call clobbers `$04-$07` (writing its own arguments into + // them) without disturbing the caller's saved parameters. + // + // Before this change, parameters lived in `$04-$07` for the + // duration of the function body, so any call nested inside + // a function's body silently corrupted the caller's + // parameters — see COMPILER_BUGS.md §2. The per-function + // RAM slots + prologue spill fix that class of bug at the + // cost of 4 LDA/STA pairs per function entry. + // + // Locals are laid out linearly across every function: + // NEScript forbids recursion (E0402) and enforces a + // bounded call depth (E0401), so lifetime overlap between + // functions is fine and we don't need to pack them. let mut local_ram_next: u16 = 0x0300; // Advance past any RAM global so locals don't clobber them. // Each global occupies `[address, address + size)` — for an @@ -236,17 +250,10 @@ impl<'a> IrCodeGen<'a> { local_ram_next = max_ram_global_end; } for func in &ir.functions { - for (i, local) in func.locals.iter().enumerate() { - if i < func.param_count { - if i < 4 { - var_addrs.insert(local.var_id, 0x04 + i as u16); - var_sizes.insert(local.var_id, local.size); - } - } else { - var_addrs.insert(local.var_id, local_ram_next); - var_sizes.insert(local.var_id, local.size); - local_ram_next += local.size.max(1); - } + for local in &func.locals { + var_addrs.insert(local.var_id, local_ram_next); + var_sizes.insert(local.var_id, local.size); + local_ram_next += local.size.max(1); } } let function_names = ir.functions.iter().map(|f| f.name.clone()).collect(); @@ -814,6 +821,36 @@ impl<'a> IrCodeGen<'a> { self.emit_label(&format!("__ir_fn_{}", func.name)); + // Prologue: spill the parameter-transport slots $04-$07 + // into the function's per-local RAM slots. The caller + // stages arguments into $04-$07 before the JSR; this + // copy makes sure that when the function body later + // makes a nested call (which clobbers $04-$07 again to + // pass its own arguments), the caller's parameters are + // still available in their dedicated RAM slots. + // + // Parameters are the first `param_count` entries of + // `func.locals`. The analyzer refuses functions with + // more than 4 parameters (E0506), so the `.take(4)` + // below is a defensive guard — it should never be hit + // in a well-formed program. + for (i, local) in func + .locals + .iter() + .take(func.param_count) + .take(4) + .enumerate() + { + if let Some(&addr) = self.var_addrs.get(&local.var_id) { + self.emit(LDA, AM::ZeroPage(0x04 + i as u8)); + if addr < 0x100 { + self.emit(STA, AM::ZeroPage(addr as u8)); + } else { + self.emit(STA, AM::Absolute(addr)); + } + } + } + // At the start of a frame handler that actually draws // sprites, clear the OAM shadow buffer so stale sprites from // the previous frame (or from a different state's handler) @@ -3598,3 +3635,73 @@ mod more_tests { ); } } + +#[test] +fn gen_function_prologue_spills_params_to_local_ram() { + // Regression test for COMPILER_BUGS.md §2: without the + // param-spill prologue, a function's parameters live only + // in the transport slots $04-$07. The first nested call + // inside the body would overwrite those slots with its + // own arguments, silently corrupting the caller's params. + // + // Compile a function that takes `x: u8`, calls `helper(x)`, + // then uses `x` again. Verify the callee reads `x` from a + // RAM slot (absolute addressing at $0300+) rather than + // directly from `$04`. + use crate::parser; + let src = r#" + game "Test" { mapper: NROM } + var out: u8 = 0 + fun helper(a: u8) -> u8 { return a } + fun caller(x: u8) -> u8 { + var tmp: u8 = helper(x) + return tmp + x + } + on frame { out = caller(42) } + start Main + "#; + let (prog, _) = parser::parse(src); + let prog = prog.unwrap(); + let analysis = crate::analyzer::analyze(&prog); + assert!( + analysis.diagnostics.iter().all(|d| !d.is_error()), + "unexpected analysis errors: {:?}", + analysis.diagnostics + ); + let ir = crate::ir::lower(&prog, &analysis); + let mut codegen = IrCodeGen::new(&analysis.var_allocations, &ir); + let insts = codegen.generate(&ir); + + // Find the __ir_fn_caller label. Immediately after it, look + // for the spill pattern: `LDA $04 / STA `. + let caller_idx = insts + .iter() + .position(|i| i.mode == AM::Label("__ir_fn_caller".into())) + .expect("caller function should be emitted"); + let mut saw_lda_zp4 = false; + let mut saw_sta_abs = false; + for inst in &insts[caller_idx + 1..] { + if let AM::Label(l) = &inst.mode { + if l.starts_with("__ir_fn_") && l != "__ir_fn_caller" { + break; + } + } + if inst.opcode == LDA && inst.mode == AM::ZeroPage(0x04) { + saw_lda_zp4 = true; + } + if saw_lda_zp4 && inst.opcode == STA { + if let AM::Absolute(a) = inst.mode { + if a >= 0x0300 { + saw_sta_abs = true; + break; + } + } + } + } + assert!( + saw_lda_zp4 && saw_sta_abs, + "caller function should open with `LDA $04 / STA ` \ + as the param-spill prologue — the fix for COMPILER_BUGS.md §2 \ + is not in effect" + ); +} diff --git a/src/ir/lowering.rs b/src/ir/lowering.rs index 768ae19..1ee9789 100644 --- a/src/ir/lowering.rs +++ b/src/ir/lowering.rs @@ -26,6 +26,14 @@ struct LoweringContext { /// symbol table). Used to decide between 8-bit and 16-bit IR /// ops for identifier reads/writes and binary operations. var_types: HashMap, + /// Current local scope prefix — mirrors the analyzer's field + /// of the same name. While lowering a function or handler + /// body this is `Some("")` (or `Some("State__frame")`, + /// etc), and `get_or_create_var` prepends + /// `"__local__{prefix}__"` to any bare identifier lookup so + /// function-local vars resolve to the scoped entry the + /// analyzer registered for them. `None` outside of any body. + current_scope_prefix: Option, next_var_id: u32, next_temp: u32, next_block: u32, @@ -98,6 +106,7 @@ impl LoweringContext { var_map, const_values: HashMap::new(), var_types, + current_scope_prefix: None, next_var_id, next_temp: 0, next_block: 0, @@ -113,13 +122,6 @@ impl LoweringContext { } } - /// Register a function parameter's type in the `var_types` map - /// so that identifier reads inside the function body know - /// whether to load as a byte or a word. - fn register_param_type(&mut self, name: &str, ty: &NesType) { - self.var_types.insert(name.to_string(), ty.clone()); - } - fn fresh_temp(&mut self) -> IrTemp { let t = IrTemp(self.next_temp); self.next_temp += 1; @@ -131,15 +133,30 @@ impl LoweringContext { format!("{prefix}_{}", self.next_block) } - fn get_or_create_var(&mut self, name: &str) -> VarId { - if let Some(&id) = self.var_map.get(name) { - id - } else { - let id = VarId(self.next_var_id); - self.next_var_id += 1; - self.var_map.insert(name.to_string(), id); - id + /// Resolve a user-written identifier to the scoped key used by + /// the symbol table. Mirrors `Analyzer::resolve_key`: tries the + /// current function/handler's qualified key first, falls back + /// to the bare key for globals / consts / enum variants / + /// state-level vars / function names. + fn scoped_key(&self, name: &str) -> String { + if let Some(prefix) = &self.current_scope_prefix { + let qualified = format!("__local__{prefix}__{name}"); + if self.var_map.contains_key(&qualified) || self.var_types.contains_key(&qualified) { + return qualified; + } } + name.to_string() + } + + fn get_or_create_var(&mut self, name: &str) -> VarId { + let key = self.scoped_key(name); + if let Some(&id) = self.var_map.get(&key) { + return id; + } + let id = VarId(self.next_var_id); + self.next_var_id += 1; + self.var_map.insert(key, id); + id } /// Recursively expand a struct-literal global initializer into @@ -408,8 +425,15 @@ impl LoweringContext { self.wide_hi.clear(); self.current_blocks = Vec::new(); self.current_locals = Vec::new(); + // Enter the function's local scope so all bare identifier + // lookups inside the body resolve against the analyzer's + // `__local__{function_name}__{name}` entries. + self.current_scope_prefix = Some(fun.name.clone()); - // Register parameters as locals + // Register parameters as locals. They're looked up via + // their bare name (which `get_or_create_var` now qualifies + // via `scoped_key`), so two different functions can each + // have a parameter named `x` without the VarIds colliding. for param in &fun.params { let var_id = self.get_or_create_var(¶m.name); self.current_locals.push(IrLocal { @@ -417,7 +441,10 @@ impl LoweringContext { name: param.name.clone(), size: type_size(¶m.param_type), }); - self.register_param_type(¶m.name, ¶m.param_type); + // Register the param type under the scoped key so + // `lower_expr` can decide 8-bit vs 16-bit loads. + let key = format!("__local__{}__{}", fun.name, param.name); + self.var_types.insert(key, param.param_type.clone()); } let entry = self.fresh_label(&format!("fn_{}_entry", fun.name)); @@ -443,21 +470,40 @@ impl LoweringContext { bank: fun.bank.clone(), source_span: fun.span, }); + self.current_scope_prefix = None; } fn lower_state(&mut self, state: &StateDecl, _is_start: bool) { - // Lower each event handler as a separate function + // Lower each event handler as a separate function. Each + // handler uses a distinct scope prefix so a `var i` in + // `Title::on frame` and one in `Playing::on frame` get + // different VarIds. if let Some(on_enter) = &state.on_enter { - self.lower_handler(&format!("{}_enter", state.name), on_enter, state); + self.lower_handler( + &format!("{}_enter", state.name), + &format!("{}__enter", state.name), + on_enter, + state, + ); } if let Some(on_exit) = &state.on_exit { - self.lower_handler(&format!("{}_exit", state.name), on_exit, state); + self.lower_handler( + &format!("{}_exit", state.name), + &format!("{}__exit", state.name), + on_exit, + state, + ); } if let Some(on_frame) = &state.on_frame { - self.lower_handler(&format!("{}_frame", state.name), on_frame, state); + self.lower_handler( + &format!("{}_frame", state.name), + &format!("{}__frame", state.name), + on_frame, + state, + ); } // Lower each scanline handler as a function named @@ -465,11 +511,12 @@ impl LoweringContext { // IRQ dispatch wrapper separately. for (line, block) in &state.on_scanline { let name = format!("{}_scanline_{line}", state.name); - self.lower_handler(&name, block, state); + let scope = format!("{}__scanline_{line}", state.name); + self.lower_handler(&name, &scope, block, state); } } - fn lower_handler(&mut self, name: &str, block: &Block, state: &StateDecl) { + fn lower_handler(&mut self, name: &str, scope_prefix: &str, block: &Block, state: &StateDecl) { self.next_temp = 0; // Same per-function reset as `lower_function`. See the // commentary there and COMPILER_BUGS.md §6 for why this is @@ -478,6 +525,7 @@ impl LoweringContext { // catastrophically wrong 16-bit IR ops. self.wide_hi.clear(); self.current_blocks = Vec::new(); + self.current_scope_prefix = Some(scope_prefix.to_string()); // Seed `current_locals` with the state's declared locals so any // `VarDecl` inside the handler body — tracked by // `lower_statement` via `current_locals` — is appended alongside @@ -488,6 +536,13 @@ impl LoweringContext { // addresses) would never see them. The result would be a // silent `LoadVar`/`StoreVar` emit-nothing bug that leaves the // temp slots uninitialized at runtime. + // + // State-level locals (declared at `state Foo { var i: u8 }` + // outside any handler) live in the GLOBAL scope so every + // handler in the state can read/write them across frames. + // `get_or_create_var` would try the scoped key first — + // which isn't registered for state-locals — then fall back + // to the bare key, which IS registered. self.current_locals = Vec::new(); for var in &state.locals { let var_id = self.get_or_create_var(&var.name); @@ -516,6 +571,7 @@ impl LoweringContext { bank: None, source_span: state.span, }); + self.current_scope_prefix = None; } fn lower_block(&mut self, block: &Block) { diff --git a/tests/emulator/goldens/arrays_and_functions.png b/tests/emulator/goldens/arrays_and_functions.png index 5032868b6729250bc501994adcf498fcbaacf3ca..dfe577a5ba7fb0a20ff291a472115ede9c4d62a0 100644 GIT binary patch delta 169 zcmV;a09OC~3H=GMtO1jt0+@g69qE(3H=m6$(CZ4)Kla|-YjUs2y(agX?7h+J3epdI zZ|*fY&Woqxytvn7?~PtlkUqKBg6fnL`e@PVXCK~k+CsaBI`0{64J11|v- X?pzPnYKQqN00000NkvXXu0mjfK0;Uc delta 177 zcmey%`ImFU8pis)%C(!mKih1`Ru${`>-fFd)xPgccV2%IH+io)NF?$7*}ZDVD?gUZ z5&u5x&EAXIT$j|p&$79jz395w692x>k6v2nf4OyUq8(6=g}&T7(~H^Mm-v%;fHDu` zCRh8)^?gq2X^gup{h{$o|6cR?y@nTFS25d72+{OdC8~0jYjPNK%47rPE--$;D86|Y bvp%DU?;8GVkL3mSG5~?6tDnm{r-UW|HriIF diff --git a/tests/emulator/goldens/platformer.audio.hash b/tests/emulator/goldens/platformer.audio.hash index f4619b8..b716f1b 100644 --- a/tests/emulator/goldens/platformer.audio.hash +++ b/tests/emulator/goldens/platformer.audio.hash @@ -1 +1 @@ -e8b3ea0b 132084 +f1b3c27b 132084 diff --git a/tests/emulator/goldens/war.audio.hash b/tests/emulator/goldens/war.audio.hash index e8f8ea6..7f338d6 100644 --- a/tests/emulator/goldens/war.audio.hash +++ b/tests/emulator/goldens/war.audio.hash @@ -1 +1 @@ -21f4a315 132084 +143660f 132084