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

Global bindings are not immutable, contrary to documentation #725

Open
geoffreyvanwyk opened this issue Jun 11, 2024 · 28 comments
Open

Global bindings are not immutable, contrary to documentation #725

geoffreyvanwyk opened this issue Jun 11, 2024 · 28 comments
Labels
bug Something isn't working question Further information is requested

Comments

@geoffreyvanwyk
Copy link
Contributor

Q A
Phel version v0.14
PHP version 8.3.7

Summary

The documentation states:

(def name meta? value)
This special form binds a value to a global symbol. A definition cannot be redefined at a later point.

Current behavior

A definition can be redefined without an exception being raised.

How to reproduce

Enter the following expressions into the REPL.

(def my-name "PHP")
(def my-name "Phel")

Expected behavior

An exception is raised with a message that my-name is already bound.

@geoffreyvanwyk geoffreyvanwyk added the bug Something isn't working label Jun 11, 2024
@Chemaclass
Copy link
Member

This is a known feature. On the REPL, defn allows you do redefine the same global binding - similar as in Clojure.
I am not sure if we want to change this behaviour; I would rather suggest update the docs to add this knowledge instead.
OK for you, @jenshaase?

@Chemaclass Chemaclass added the question Further information is requested label Jun 11, 2024
@geoffreyvanwyk
Copy link
Contributor Author

This is a known feature. On the REPL, defn allows you do redefine the same global binding - similar as in Clojure.

In this case, it is just def, not defn.

@t-matsudate
Copy link
Contributor

t-matsudate commented Jun 17, 2024

Hi, I'm @t-matsudate.

I consider this occurs because the def symbol analyzer allows any duplicates of namespace/name pairs.

$this->analyzer->addDefinition($namespace, $nameSymbol);

Therefore, I'd like to propose following code to resolve this issue.

// DefSymbol.php
if (!$this->analyzer->hasDefinition($namespace, $nameSymbol->getName())) {
    $this->analyzer->addDefinition($namespace, $nameSymbol);
} else {
    throw AnalyzerException::definitionDuplication(...);
}

However I've not been sure specification of the Phel yet.
So I wish to hear everyone's consideration of this issue.

@Chemaclass
Copy link
Member

Oh, good spot. Now I get what you mean. Do you mind opening a PR with your proposal, I think that should be correct in order to preserving immutability of the definitions. @jenshaase, was it some reason why we have this behaviour that I cannot recall? Or we just simply missed it?

@t-matsudate
Copy link
Contributor

@Chemaclass Okay. please wait some moment. I'll create PR after implementing it with tests.

@smeghead
Copy link
Member

I sometimes use REPL to test the behavior of a function.
If def or defn cannot be redefined, I have to rerun REPL many times.
Wouldn't it be more convenient to be able to redefine def and defn at least when running REPL?

@Chemaclass
Copy link
Member

The thing is that this is also the behaviour wen executing run, and I assumed that this shouldn't be possible, right?
Screenshot 2024-06-17 at 15 20 39

@t-matsudate
Copy link
Contributor

@smeghead Although this is case in the Clojure, that has several way to redefine vars instead of reusing def:

This changes root binding of its var while keeping the namespace/name symbol.
However we should use this in some limited case because this FORCIBLY changes root bindging. this makes callers confused until with-redefs's scope got exited.

This is same as with-redefs except this uses any updater function.

This is the thread local binding in the Clojure.
Probably this is the most safe way.

Otherwise the Clojure requires us that switchs current namespace.

@t-matsudate
Copy link
Contributor

@Chemaclass Yes. And also I'm thinking its rule shouldn't easily be changed between repl and run.

@t-matsudate
Copy link
Contributor

I've written following code to resolve this issue:

// DefSymbol.php
        if (!$this->analizer->hasDefinition($namespace, $nameSymbol->getName())) {
            $this->analyzer->addDefinition($namespace, $nameSymbol);
        } else {
            throw AnalyzerException::definitionDuplication($list, $namespace, $nameSymbol);
        }
// AnalyzerException.php
    public static function definitionDuplication(
        PersistentListInterface $list,
        string $namespace,
        Symbol $nameSymbol
    ): self {
        return self::withLocation(
            sprintf(
                'Var "%s"\\"%s" is already defined.',
                $namespace,
                $nameSymbol->getName(),
            ),
            $list,
        );
    }

Then php-cs-fixer noticed above simple file changes as an error.

~/phel-lang$ composer test
> vendor/bin/psalm --clear-cache
Cache directory deleted
> vendor/bin/phpstan clear-result-cache
Note: Using configuration file /home/runner/phel-lang/phpstan.neon.
Result cache cleared from directory:
/tmp/phpstan
> ./vendor/bin/php-cs-fixer fix --dry-run
PHP CS Fixer 3.58.1 7th Gear by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.3.0
Running analysis on 1 core sequentially.
You can enable parallel runner and speed up the analysis! Please see usage docs for more information.
Loaded config default from "/home/runner/phel-lang/.php-cs-fixer.dist.php".
Using cache file ".php-cs-fixer.cache".
 592/592 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

   1) src/php/Compiler/Domain/Analyzer/Exceptions/AnalyzerException.php

Found 1 of 592 files that can be fixed in 0.160 seconds, 20.00 MB memory used
Script ./vendor/bin/php-cs-fixer fix --dry-run handling the csrun event returned with error code 8
Script @csrun was called via test-quality
Script @test-quality was called via test-all
Script @test-all was called via test

Could you tell me how do I cope with this thing?

@Chemaclass
Copy link
Member

Have you tried "composer fix" first?

@t-matsudate
Copy link
Contributor

t-matsudate commented Jun 18, 2024

Sorry, I was forgetting it and ran composer fix.
Then I got the message to mean of no error from php-cs-fixer. 😄

@t-matsudate
Copy link
Contributor

t-matsudate commented Jun 18, 2024

After above, I added following fixes:

// AnalyzerInterface.php
    public function hasDefinition(string $ns, Symbol $name): bool;
// Analyzer.php
    public function hasDefinition(string $ns, Symbol $name): bool
    {
        return $this->globalEnvironment->hasDefinition($ns, $name);
    }

And I ran composer test once more. Then I got following errors:

Cache directory deleted
Result cache cleared from directory:
/tmp/phpstan

Found 0 of 592 files that can be fixed in 0.027 seconds, 20.00 MB memory used
------------------------------
�[42;2m                              �[0m
�[42;30;2m       No errors found!       �[0m
�[42;2m                              �[0m
------------------------------
Psalm can automatically fix 13 of these issues.
Run Psalm again with 
�[30;48;5;195m--alter --issues=MissingParamType --dry-run�[0m
to see what it can fix.
------------------------------

Checks took 10.32 seconds and used 144.240MB of memory
Psalm was able to infer types for 97.8284% of the codebase

 [OK] No errors                                                                 



 [OK] Rector is done!                                                           

PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.0
Configuration: /home/runner/phel-lang/phpunit.xml.dist
Random Seed:   1718699591

...............................FEEEEEEEEE...RRREFFEE...........  63 / 914 (  6%)
....................................................F.....SFS.F 126 / 914 ( 13%)
[2024-06-18T08:33:12+00:00] Error Unknown(-1) found!
message: "file_get_contents(/home/runner/phel-lang/tests/php/Integration/Build/Command/out/main.php): Failed to open stream: No such file or directory"
file: /home/runner/phel-lang/tests/php/Integration/Build/Command/BuildCommandTest.php:107
context: {"errno":2}
............................................................... 189 / 914 ( 20%)
............................................................... 252 / 914 ( 27%)
............................................................... 315 / 914 ( 34%)
............................................................... 378 / 914 ( 41%)
................................................Phel\Compiler\Domain\Analyzer\Exceptions\AnalyzerException: .
............... 441 / 914 ( 48%)
............................................................... 504 / 914 ( 55%)
............................................................... 567 / 914 ( 62%)
............................................................... 630 / 914 ( 68%)
............................................................... 693 / 914 ( 75%)
............................................................... 756 / 914 ( 82%)
...........................................

Time: 00:01.507, Memory: 28.00 MB

There were 12 errors:

1) PhelTest\Integration\Run\Command\Repl\ReplCommandTest::test_integration_with_core_lib with data set "require-namespace.test" ('phel:1> (require phel\html :a...div>\n', PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...))
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:97
/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:68
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:57
/home/runner/phel-lang/src/php/Build/Application/FileEvaluator.php:25
/home/runner/phel-lang/src/php/Build/BuildFacade.php:102
/home/runner/phel-lang/src/php/Run/RunFacade.php:69
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:128
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:97
/home/runner/phel-lang/vendor/symfony/console/Command/Command.php:279
/home/runner/phel-lang/tests/php/Integration/Run/Command/Repl/ReplCommandTest.php:74

2) PhelTest\Integration\Run\Command\Repl\ReplCommandTest::test_integration with data set "undefined-symbol.test" ('phel:1> (def my-fn (fn []\n.....^^\n\n', PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...))
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:97
/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:68
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:57
/home/runner/phel-lang/src/php/Build/Application/FileEvaluator.php:25
/home/runner/phel-lang/src/php/Build/BuildFacade.php:102
/home/runner/phel-lang/src/php/Run/RunFacade.php:69
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:128
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:97
/home/runner/phel-lang/vendor/symfony/console/Command/Command.php:279
/home/runner/phel-lang/tests/php/Integration/Run/Command/Repl/ReplCommandTest.php:51

3) PhelTest\Integration\Run\Command\Repl\ReplCommandTest::test_integration with data set "unterminated-list-multiline-with-empty-lines.test" ('phel:1> (php/+ 1\n....:2>\n.....   ^\n', PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...))
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:97
/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:68
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:57
/home/runner/phel-lang/src/php/Build/Application/FileEvaluator.php:25
/home/runner/phel-lang/src/php/Build/BuildFacade.php:102
/home/runner/phel-lang/src/php/Run/RunFacade.php:69
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:128
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:97
/home/runner/phel-lang/vendor/symfony/console/Command/Command.php:279
/home/runner/phel-lang/tests/php/Integration/Run/Command/Repl/ReplCommandTest.php:51

4) PhelTest\Integration\Run\Command\Repl\ReplCommandTest::test_integration with data set "simple-add.test" ('phel:1> (php/+ 1 1)\n2\n', PhelTest\Integration\Run\Command\Repl\InputLine Object (...))
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:97
/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:68
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:57
/home/runner/phel-lang/src/php/Build/Application/FileEvaluator.php:25
/home/runner/phel-lang/src/php/Build/BuildFacade.php:102
/home/runner/phel-lang/src/php/Run/RunFacade.php:69
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:128
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:97
/home/runner/phel-lang/vendor/symfony/console/Command/Command.php:279
/home/runner/phel-lang/tests/php/Integration/Run/Command/Repl/ReplCommandTest.php:51

5) PhelTest\Integration\Run\Command\Repl\ReplCommandTest::test_integration with data set "simple-add-multiline.test" ('phel:1> (php/+ 1\n....:2> 2\n...)\n5\n', PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...))
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:97
/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:68
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:57
/home/runner/phel-lang/src/php/Build/Application/FileEvaluator.php:25
/home/runner/phel-lang/src/php/Build/BuildFacade.php:102
/home/runner/phel-lang/src/php/Run/RunFacade.php:69
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:128
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:97
/home/runner/phel-lang/vendor/symfony/console/Command/Command.php:279
/home/runner/phel-lang/tests/php/Integration/Run/Command/Repl/ReplCommandTest.php:51

6) PhelTest\Integration\Run\Command\Repl\ReplCommandTest::test_integration with data set "exit-multiline-mode-with-ctrl-d.test" ('phel:1> (php/+ 1\n....:2> 2\n...)\n3\n', PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...))
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:97
/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:68
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:57
/home/runner/phel-lang/src/php/Build/Application/FileEvaluator.php:25
/home/runner/phel-lang/src/php/Build/BuildFacade.php:102
/home/runner/phel-lang/src/php/Run/RunFacade.php:69
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:128
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:97
/home/runner/phel-lang/vendor/symfony/console/Command/Command.php:279
/home/runner/phel-lang/tests/php/Integration/Run/Command/Repl/ReplCommandTest.php:51

7) PhelTest\Integration\Run\Command\Repl\ReplCommandTest::test_integration with data set "unterminated-list-multiline.test" ('phel:1> (php/+ 1\n....:2> 2\n...   ^\n', PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...), PhelTest\Integration\Run\Command\Repl\InputLine Object (...))
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:97
/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:68
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:57
/home/runner/phel-lang/src/php/Build/Application/FileEvaluator.php:25
/home/runner/phel-lang/src/php/Build/BuildFacade.php:102
/home/runner/phel-lang/src/php/Run/RunFacade.php:69
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:128
/home/runner/phel-lang/src/php/Run/Infrastructure/Command/ReplCommand.php:97
/home/runner/phel-lang/vendor/symfony/console/Command/Command.php:279
/home/runner/phel-lang/tests/php/Integration/Run/Command/Repl/ReplCommandTest.php:51

8) PhelTest\Integration\Phel\PhelTest::test_globals_argv_as_string_via_run
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:97
/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:68
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:57
/home/runner/phel-lang/src/php/Build/Application/FileEvaluator.php:25
/home/runner/phel-lang/src/php/Build/BuildFacade.php:102
/home/runner/phel-lang/src/php/Run/Application/NamespaceRunner.php:32
/home/runner/phel-lang/src/php/Run/RunFacade.php:23
/home/runner/phel-lang/src/php/Phel.php:38
/home/runner/phel-lang/tests/php/Integration/Phel/PhelTest.php:26

9) PhelTest\Integration\Phel\PhelTest::test_globals_argv_as_array_via_run
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:97
/home/runner/phel-lang/src/php/Compiler/Application/EvalCompiler.php:68
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:57
/home/runner/phel-lang/src/php/Build/Application/FileEvaluator.php:25
/home/runner/phel-lang/src/php/Build/BuildFacade.php:102
/home/runner/phel-lang/src/php/Run/Application/NamespaceRunner.php:32
/home/runner/phel-lang/src/php/Run/RunFacade.php:23
/home/runner/phel-lang/src/php/Phel.php:38
/home/runner/phel-lang/tests/php/Integration/Phel/PhelTest.php:34

10) PhelTest\Integration\IntegrationTest::test_integration
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined. in /home/runner/phel-lang/src/php/Compiler/Application/CodeCompiler.php:102
Stack trace:
#0 /home/runner/phel-lang/src/php/Compiler/Application/CodeCompiler.php(69): Phel\Compiler\Application\CodeCompiler->analyze(Object(Phel\Compiler\Domain\Parser\ReadModel\ReaderResult))
#1 /home/runner/phel-lang/src/php/Compiler/CompilerFacade.php(88): Phel\Compiler\Application\CodeCompiler->compileString('(ns phel\\core\n ...', Object(Phel\Compiler\Infrastructure\CompileOptions))
#2 /home/runner/phel-lang/src/php/Build/Application/FileCompiler.php(33): Phel\Compiler\CompilerFacade->compile('(ns phel\\core\n ...', Object(Phel\Compiler\Infrastructure\CompileOptions))
#3 /home/runner/phel-lang/src/php/Build/BuildFacade.php(90): Phel\Build\Application\FileCompiler->compileFile('/home/runner/ph...', '/tmp/phel-coree...', true)
#4 /home/runner/phel-lang/tests/php/Integration/IntegrationTest.php(31): Phel\Build\BuildFacade->compileFile('/home/runner/ph...', '/tmp/phel-coree...')
#5 /home/runner/phel-lang/vendor/phpunit/phpunit/src/Framework/TestSuite.php(629): PhelTest\Integration\IntegrationTest::setUpBeforeClass()
#6 /home/runner/phel-lang/vendor/phpunit/phpunit/src/Framework/TestSuite.php(685): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#7 /home/runner/phel-lang/vendor/phpunit/phpunit/src/Framework/TestSuite.php(685): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#8 /home/runner/phel-lang/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(651): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#9 /home/runner/phel-lang/vendor/phpunit/phpunit/src/TextUI/Command.php(146): PHPUnit\TextUI\TestRunner->run(Object(PHPUnit\Framework\TestSuite), Array, Array, true)
#10 /home/runner/phel-lang/vendor/phpunit/phpunit/src/TextUI/Command.php(99): PHPUnit\TextUI\Command->run(Array, true)
#11 /home/runner/phel-lang/vendor/phpunit/phpunit/phpunit(107): PHPUnit\TextUI\Command::main()
#12 /home/runner/phel-lang/vendor/bin/phpunit(122): include('/home/runner/ph...')
#13 {main}
11) PhelTest\Integration\Interop\CallPhelTest::test_call_odd
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/CodeCompiler.php:102
/home/runner/phel-lang/src/php/Compiler/Application/CodeCompiler.php:69
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:88
/home/runner/phel-lang/src/php/Build/Application/FileCompiler.php:33
/home/runner/phel-lang/src/php/Build/BuildFacade.php:90
/home/runner/phel-lang/tests/php/Integration/Interop/CallPhelTest.php:22

12) PhelTest\Integration\Interop\CallPhelTest::test_call_print_str
Phel\Compiler\Domain\Exceptions\CompilerException: Var "phel\core"\"*ns*" is already defined.

/home/runner/phel-lang/src/php/Compiler/Application/CodeCompiler.php:102
/home/runner/phel-lang/src/php/Compiler/Application/CodeCompiler.php:69
/home/runner/phel-lang/src/php/Compiler/CompilerFacade.php:88
/home/runner/phel-lang/src/php/Build/Application/FileCompiler.php:33
/home/runner/phel-lang/src/php/Build/BuildFacade.php:90
/home/runner/phel-lang/tests/php/Integration/Interop/CallPhelTest.php:22

--

There were 6 failures:

1) PhelTest\Integration\Interop\Command\Export\ExportCommandTest::test_export_command_multiple
Failed asserting that file "/home/runner/phel-lang/tests/php/Integration/Interop/Command/Export/PhelGenerated/TestCmdExportMultiple/Adder.php" exists.

/home/runner/phel-lang/tests/php/Integration/Interop/Command/Export/ExportCommandTest.php:42

2) PhelTest\Integration\Run\Command\Test\TestCommandProjectSuccess\TestCommandProjectSuccessTest::test_all_in_project
Failed asserting that '�[33;34mVar "phel\core"\"*ns*" is already defined.�[0m\n
in /home/runner/phel-lang/src/phel/core.phel:38\n
\n
38| (def *ns*\n
39|   "Returns the namespace in the current scope."\n
40|   "\\phel\\core")\n
\n
' matches PCRE pattern "/.*Total: 2.*/".

3) PhelTest\Integration\Run\Command\Test\TestCommandProjectSuccess\TestCommandProjectSuccessTest::test_one_file_in_project
Failed asserting that '�[33;34mVar "phel\core"\"*ns*" is already defined.�[0m\n
in /home/runner/phel-lang/src/phel/core.phel:38\n
\n
38| (def *ns*\n
39|   "Returns the namespace in the current scope."\n
40|   "\\phel\\core")\n
\n
' matches PCRE pattern "/.*Total: 1.*/".

4) PhelTest\Integration\Run\Command\Test\TestCommandProjectFailure\TestCommandProjectFailureTest::test_all_in_failed_project
Failed asserting that '�[33;34mVar "phel\core"\"*ns*" is already defined.�[0m\n
in /home/runner/phel-lang/src/phel/core.phel:38\n
\n
38| (def *ns*\n
39|   "Returns the namespace in the current scope."\n
40|   "\\phel\\core")\n
\n
' matches PCRE pattern "/.*Total: 1.*/".

5) PhelTest\Integration\Build\Command\BuildCommandTest::test_build_project
Failed asserting that '' matches PCRE pattern "/This is printed from no-cache.phel/".

/home/runner/phel-lang/tests/php/Integration/Build/Command/BuildCommandTest.php:47

6) PhelTest\Integration\Build\Command\BuildCommandTest::test_out_main_file
Failed asserting that false is identical to '<?php declare(strict_types=1);\n
\n
require_once dirname(__DIR__) . "/vendor/autoload.php";\n
\n
$compiledFile = __DIR__ . "/test_ns/hello.php";\n
\n
require_once $compiledFile;'.

/home/runner/phel-lang/tests/php/Integration/Build/Command/BuildCommandTest.php:117

--

There were 3 risky tests:

1) PhelTest\Integration\Run\Command\Run\RunCommandTest::test_file_not_found
Test code or tested code did not (only) close its own output buffers

2) PhelTest\Integration\Run\Command\Run\RunCommandTest::test_run_by_filename
Test code or tested code did not (only) close its own output buffers

3) PhelTest\Integration\Run\Command\Run\RunCommandTest::test_run_by_namespace
Test code or tested code did not (only) close its own output buffers

--

There were 2 skipped tests:

1) PhelTest\Integration\Api\ApiFacadeTest::test_number_of_grouped_functions
Useful to debug the facade, but useless to keep it in the CI

/home/runner/phel-lang/tests/php/Integration/Api/ApiFacadeTest.php:23

2) PhelTest\Integration\Build\Command\BuildCommandTest::test_build_project_cached
This test depends on "PhelTest\Integration\Build\Command\BuildCommandTest::test_build_project" to pass.

ERRORS!
Tests: 914, Assertions: 1285, Errors: 12, Failures: 6, Skipped: 2, Risky: 3.

This is looked seem the Phel's def redefines *ns* whenever we use itself.
Now I wanna confirm behavior of def in the PHP's source code.
Could you tell me what happend at this moment in the Phel?

@Chemaclass
Copy link
Member

Do you mind creating a PR to be able to see the full diff and the CI outcome as well, @t-matsudate ? It's going to be easy to keep track on the progress and changes there

@t-matsudate
Copy link
Contributor

I would send PR for this but couldn't commit changes because composer test starts at precommit timing.
What the way to skip test only this moment are there?
Or should I cancel these changes?

@Chemaclass
Copy link
Member

@t-matsudate you can commit using the --no-verify flag to skip the git hooks

eg: git commit -m 'your message' --no-verify

@t-matsudate
Copy link
Contributor

Thank you for telling good news to me!
However I've succeeded commit to perform another way.
Therefore I'll create PR for above to this repository soon.

@love-your-parens
Copy link

Hello! Apologies for butting in, but wouldn't this fix entail a regression in functionality? The ability to redefine bindings is fundamental to REPL driven development in Lisps, including Clojure. It might be better to change the spec, not the implementation.

@t-matsudate
Copy link
Contributor

@love-your-parens
Thnks for joining this discussion.
But I wish to make your comment more clear.

It might be better to change the spec, not the implementation.

This is:

  • I/We should fix Phel-side code, not PHP-side.
  • I/We should revise just Phel's documentation.

Could you tell me which of above does your comment indicate?
Or do you tell about more different thing?

@love-your-parens
Copy link

@t-matsudate
Sure! It's the latter: I think the current behaviour is desireable (for practical reasons), and the documentation should be revised to reflect it.

@t-matsudate
Copy link
Contributor

@love-your-parens
I understood your comment, then I'll swtich my work into document revisions.
Thank you for the important notice! :)

@t-matsudate
Copy link
Contributor

@Chemaclass @smeghead @love-your-parens @geoffreyvanwyk
I worked documentation fix as phel-lang/phel-lang.org#83.

@jenshaase
Copy link
Member

Hi all,

my initial thoughts were exactly what the documentations says "A definition cannot be redefined at a later point.". From a theoretical point this still makes sense for me today. Redefining a definition makes program code much more hard to read because you cannot easily understand what is going on.

However, I cannot remember anymore why I didn't implemented it that way. Maybe I just forget it or I had a problem with it and wanted to fix it later.

Changing the definition in the documentation has the drawback that def and var are then basically the same which is also not ideal.

If someone has the time and the will to implemented the immutability of def I would be very happy. If this is no the case I can accept that we change the documentation as request in the PR.

What do you think?

@Chemaclass
Copy link
Member

TLDR: I would consider this a "bug" that needs to be fixed rather than changing the docs.

I prefer to aim for immutability for def (except for the REPL for pragmatic reasons of quick/easy feedback on re-definitions if needed). Otherwise, as Jens says, we would lose the immutability core principle (having def and var with the same meaning...).

@smeghead
Copy link
Member

Hi @jenshaase @Chemaclass
This is my opinion. (again)

I believe that REPL is an important feature for the Lisp family of programming languages.

When I use phel for various programming, I basically practice Test Driven Development, but I also use REPL to redefine functions dynamically to create the desired function while checking its behavior.

If the REPL cannot be used to redefine def, it will be necessary to restart the REPL each time, which I feel will be a little less convenient.

@Chemaclass
Copy link
Member

@smeghead yes, totally agree. So the goal should be

  1. allow redefine def in the REPL for pragmatic reasons
  2. aim for immutability for def on non-REPL execution

In this case, the 2. is clearly not working, so it's a bug that needs a little attention; an initial WIP PR to fix it #726

@jasalt
Copy link

jasalt commented Oct 31, 2024

Being familiar with Clojure, the defonce can be used for defs that cannot be later changed. It is seen used often eg. during REPL development to avoid re-defining certain vars when re-evaluating file after changes.

Without much perspective from other Lisps, I would expect Phel to behave similar way. Seems supported in Basilisp also.

@Chemaclass
Copy link
Member

Thanks for the feedback, @jasalt! I think that's a very good idea and a good compromise with the existing behaviour. What do you think, @jenshaase ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

7 participants