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

Prevent user from declaring variables and functions with same name #1992

Merged
merged 2 commits into from
Sep 24, 2022

Conversation

pramodk
Copy link
Member

@pramodk pramodk commented Sep 23, 2022

  • Today user can "declare" a variable in the NEURON block and then PROCEDURE or FUNCTION with the same name. For example, the below doesn't raise any error today:

    NEURON {
        SUFFIX glia
        RANGE alfa
    }
    
    FUNCTION alfa(v(mV)) {
        alfa = exp(v)
    }
  • The reason this works is that the RANGE alfa declaration in the NEURON block is not complete (i.e. variable needs to be "defined" in blocks like ASSIGNED, PARAMETER, etc) and hence doesn't appear anywhere in the generated code. If we try to use alfa as a variable then that is a warning and resulted in a compilation error.

  • This behavior kind of works but is error-prone. Apart from corner cases and confusing behavior, it's difficult to do certain analyses that we would like to do (e.g. in new NMODL code generation).

  • So for robustness and simplicity, we now throw an error if a user tries to declare a variable and function with the same name.

        $ ./bin/nocmodl mod_x/test.mod Translating mod_x/test.mod into mod_x/test.cpp
            Error: alfa used as both variable and function in file mod/test.mod 

ModelDB PRs

* Today use can "declarae" a variable in NEURON block and
  then PROCEDURE or FUNCTION with the same name. For example,
  below doesn't raise any error today:

   ```python
   NEURON {
       SUFFIX glia
       RANGE alfa
   }

   FUNCTION alfa(v(mV)) {
       alfa = exp(v)
   }
   ```

* The reason this works is that the `RANGE alfa` declaration in
  NEURON block id not complete (i.e. variable needs to be "defined"
  in block like ASSIGNED, PARAMETER etc) and hence doesn't appear
  anywhere in the generated code. If we try to use `alfa` as variable
  then that is warning and resulted into compilation error.

* This behaviour kind of works but error prone. Apart from corner cases
  and confusing behaviour, it's difficult to do certain analysis that
  we would like to do (e.g. in new NMODL code generation).

* So for the robustness and simplicity, we now throw an error if user
  try to declarte variable and function with the same name.

  ```console
   $ ./bin/nocmodl mod_x/test.mod
       Translating mod_x/test.mod into mod_x/test.cpp
       Error: alfa used as both variable and function in file mod/test.mod
  ```
@azure-pipelines
Copy link

✔️ 7c4660b -> Azure artifacts URL

Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. should raise error.

pramodk added a commit to neuronsimulator/testcorenrn that referenced this pull request Sep 23, 2022
pramodk added a commit to neuronsimulator/reduced_dentate that referenced this pull request Sep 23, 2022
pramodk added a commit to neuronsimulator/reduced_dentate that referenced this pull request Sep 23, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1992 (f0fe629) into master (d2564ff) will increase coverage by 0.00%.
The diff coverage is 76.92%.

@@           Coverage Diff           @@
##           master    #1992   +/-   ##
=======================================
  Coverage   46.48%   46.48%           
=======================================
  Files         527      527           
  Lines      119181   119191   +10     
=======================================
+ Hits        55402    55410    +8     
- Misses      63779    63781    +2     
Impacted Files Coverage Δ
src/nmodl/io.cpp 65.06% <0.00%> (ø)
src/nmodl/nocpout.cpp 93.25% <ø> (ø)
src/nmodl/consist.cpp 79.59% <83.33%> (+0.10%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pramodk added a commit to pramodk/dentate that referenced this pull request Sep 23, 2022
release (current master)

* MOD file translation is being changed from C to C++ and requires
  certain changes for type safety. These are all syntax changes.
  See https://nrn.readthedocs.io/en/latest/guide/porting_mechanisms_to_cpp.html

* Gfluct3.mod declares a variable of the same name as RANGE variable.
  This will be an error in the upcoming release.
  See neuronsimulator/nrn/pull/1992
@azure-pipelines
Copy link

✔️ f0fe629 -> Azure artifacts URL

@pramodk
Copy link
Member Author

pramodk commented Sep 23, 2022

ModelDB models / mod files that needs to be fixed are:

nrn-modeldb-ci git:(master) ✗ grep "both variable and function in file" log
Error: new_seed used as both variable and function in file ./cache/138205/Schizophr/Gfluct.mod
Error: new_seed used as both variable and function in file ./cache/155601/ProdduturEtAl2013/Gfluct2.mod
Error: new_seed used as both variable and function in file ./cache/190559/MiglioreEJN2016/Gfluct.mod
Error: minf used as both variable and function in file ./cache/139657/Kopp-Scheinpflug2011/lva.mod
Error: new_seed used as both variable and function in file ./cache/139657/Kopp-Scheinpflug2011/mhh_Gfluct.mod
Error: new_seed used as both variable and function in file ./cache/124513/dentate_gyrus/Gfluct2.mod
Error: new_seed used as both variable and function in file ./cache/262115/demo_destexhe-pare-1999/corrgen8.mod
Error: r used as both variable and function in file ./cache/139656/network/Golgi_hcn1.mod
Error: alfa used as both variable and function in file ./cache/139656/network/GRC_NA.mod
Error: diffusione used as both variable and function in file ./cache/116835/GrC/GRC_GABA.mod
Error: alfa used as both variable and function in file ./cache/116835/GrC/GRC_NA.mod
Error: new_seed used as both variable and function in file ./cache/232876/TCR/high_conductance_state/Gfluct.mod
Error: new_seed used as both variable and function in file ./cache/240116/modeldb-bulb3d/sim/Gfluct.mod
Error: new_seed used as both variable and function in file ./cache/217882/LombardoHarrington2016/Gfluctdv.mod
Error: curr used as both variable and function in file ./cache/151825/Demo/epsp.mod
Error: new_seed used as both variable and function in file ./cache/97917/splitcell/pardentategyrus/Gfluct2.mod
Error: new_seed used as both variable and function in file ./cache/183949/Discharge_hysteresis/Gfluctdv.mod
Error: new_seed used as both variable and function in file ./cache/155602/YuEtAl2013/Gfluct2.mod
Error: r used as both variable and function in file ./cache/127021/Golgi_cell_NaKATPAse/Golgi_hcn1.mod
Error: new_seed used as both variable and function in file ./cache/143671/PowersEtAl2012/code/Gfluctdv.mod
Error: new_seed used as both variable and function in file ./cache/144482/Pyramidal_STDP_Gomez_Delgado_2010/mechanism/mechanism_cell1/Gfluct.mod
Error: minf used as both variable and function in file ./cache/53893/ca_t_lva/lva.mod
Error: alfa used as both variable and function in file ./cache/265584/Granule_cell_2020/04_GrC_2020_accelerating/mod_files/GRC_NA_FHF.mod
Error: alfa used as both variable and function in file ./cache/265584/Granule_cell_2020/04_GrC_2020_accelerating/mod_files/GRC_NA.mod
Error: alfa used as both variable and function in file ./cache/265584/Granule_cell_2020/02_GrC_2020_mild_adapting/mod_files/GRC_NA_FHF.mod
Error: alfa used as both variable and function in file ./cache/265584/Granule_cell_2020/02_GrC_2020_mild_adapting/mod_files/GRC_NA.mod
Error: alfa used as both variable and function in file ./cache/265584/Granule_cell_2020/03_GrC_2020_adapting/mod_files/GRC_NA_FHF.mod
Error: alfa used as both variable and function in file ./cache/265584/Granule_cell_2020/03_GrC_2020_adapting/mod_files/GRC_NA.mod
Error: alfa used as both variable and function in file ./cache/265584/Granule_cell_2020/01_GrC_2020_regular/mod_files/GRC_NA_FHF.mod
Error: alfa used as both variable and function in file ./cache/265584/Granule_cell_2020/01_GrC_2020_regular/mod_files/GRC_NA.mod
Error: r used as both variable and function in file ./cache/232023/cerebellar_granular_network/mechanisms/Golgi_CL/Golgi_hcn1.mod
Error: new_seed used as both variable and function in file ./cache/150024/CNModel_May2013/Ifluct8.mod
Error: new_seed used as both variable and function in file ./cache/122329/GPSingleCompartment/syn.mod
Error: new_seed used as both variable and function in file ./cache/141273/Vierling-ClaassenEtAl2010/Gfluct.mod
Error: new_seed used as both variable and function in file ./cache/83590/Arsiero_et_al2007/mechanisms/Ifluct1.mod

@pramodk pramodk marked this pull request as draft September 23, 2022 14:22
Helveg pushed a commit to dbbs-lab/dbbs-mod-collection that referenced this pull request Sep 23, 2022
pramodk added a commit to ModelDBRepository/138205 that referenced this pull request Sep 24, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
pramodk added a commit to ModelDBRepository/155601 that referenced this pull request Sep 24, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
pramodk added a commit to ModelDBRepository/190559 that referenced this pull request Sep 24, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
pramodk added a commit to ModelDBRepository/139657 that referenced this pull request Sep 24, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
pramodk added a commit to ModelDBRepository/124513 that referenced this pull request Sep 24, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
pramodk added a commit to ModelDBRepository/262115 that referenced this pull request Sep 24, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
pramodk added a commit to ModelDBRepository/139656 that referenced this pull request Sep 24, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
@pramodk pramodk marked this pull request as ready for review September 24, 2022 10:22
@pramodk
Copy link
Member Author

pramodk commented Sep 24, 2022

As PRs to respective ModelDB models are created, I will merge this.

@pramodk pramodk merged commit 02574e7 into master Sep 24, 2022
@pramodk pramodk deleted the pramodk/avoid-var-function-conflict branch September 24, 2022 10:28
ramcdougal pushed a commit to ModelDBRepository/155601 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/190559 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/139657 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/124513 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/139656 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/116835 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/232876 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/240116 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/217882 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/151825 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/97917 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/183949 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/155602 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/127021 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/143671 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/144482 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/53893 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/265584 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/232023 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/150024 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/122329 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/141273 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal pushed a commit to ModelDBRepository/83590 that referenced this pull request Nov 4, 2022
* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992
ramcdougal added a commit to ModelDBRepository/262115 that referenced this pull request Nov 4, 2022
* Update MOD files to avoid conflicting variable and function name

* This will be an error in the upcoming NEURON 9.0 release
* For details, see neuronsimulator/nrn#1992

* Update README.html

Co-authored-by: ramcdougal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants