Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP: top-level type cannot be named kaitai_struct #1126

Open
generalmimon opened this issue Aug 18, 2024 · 2 comments
Open

PHP: top-level type cannot be named kaitai_struct #1126

generalmimon opened this issue Aug 18, 2024 · 2 comments

Comments

@generalmimon
Copy link
Member

generalmimon commented Aug 18, 2024

I ran into some suspicious code again - PHPTranslator.scala:156-161:

  def types2classAbs(names: List[String]) =
    names match {
      case List("kaitai_struct") => PHPCompiler.kstructName
      case _ =>
        namespaceRef + "\\" + PHPCompiler.types2classRel(names)
    }

Is this some kind of easter egg? In terms of language design, why couldn't a user name the top-level type kaitai_struct?

A quick test confirms that kaitai_struct as /meta/id really breaks things for PHP:

kaitai_struc.ksy

meta:
  id: kaitai_struc
seq:
  - id: recursive
    type: kaitai_struc
    if: false

kaitai_struct.ksy

meta:
  id: kaitai_struct
seq:
  - id: recursive
    type: kaitai_struct
    if: false
$ kaitai-struct-compiler -- --verbose file -t php kaitai_struc.ksy kaitai_struct.ksy
Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=UTF-8
parsing kaitai_struc.ksy...
reading kaitai_struc.ksy...
... compiling it for php...
.... writing KaitaiStruc.php
parsing kaitai_struct.ksy...
reading kaitai_struct.ksy...
... compiling it for php...
.... writing KaitaiStruct.php

Note: using KSC in revision 3c855b38

diff --git 1/KaitaiStruc.php 2/KaitaiStruct.php
index 76e786e..678c30d 100644
--- 1/KaitaiStruc.php
+++ 2/KaitaiStruct.php
@@ -2,15 +2,15 @@
 // This is a generated file! Please edit source .ksy file and use kaitai-struct-compiler to rebuild
 
 namespace {
-    class KaitaiStruc extends \Kaitai\Struct\Struct {
-        public function __construct(\Kaitai\Struct\Stream $_io, \Kaitai\Struct\Struct $_parent = null, \KaitaiStruc $_root = null) {
+    class KaitaiStruct extends \Kaitai\Struct\Struct {
+        public function __construct(\Kaitai\Struct\Stream $_io, \Kaitai\Struct\Struct $_parent = null, \Kaitai\Struct\Struct $_root = null) {
             parent::__construct($_io, $_parent, $_root === null ? $this : $_root);
             $this->_read();
         }
 
         private function _read() {
             if (false) {
-                $this->_m_recursive = new \KaitaiStruc($this->_io, $this, $this->_root);
+                $this->_m_recursive = new \Kaitai\Struct\Struct($this->_io, $this, $this->_root);
             }
         }
         protected $_m_recursive;
@generalmimon
Copy link
Member Author

generalmimon commented Aug 18, 2024

I think the case List("kaitai_struct") => branch is actually never used for the stated purpose (the only scenario in which it is used is when someone names the top-level type kaitai_struct as I did), because this is one of the only 2 occurrences of "kaitai_struct" in the compiler codebase (the other one in RustTranslator.scala looks basically the same). So it's probably a legacy of older code and is no longer used.

@GreyCat
Copy link
Member

GreyCat commented Aug 18, 2024

Totally agree. Moreover, I would contest removing just about anything in the codebase which does not result in any regression in the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants