Opened 8 years ago

Last modified 3 years ago

#3535 assigned defect

duplicate function names are not detected

Reported by: wbraun Owned by: adrpo
Priority: blocker Milestone: 1.19.0
Component: Interactive Environment Version:
Keywords: Cc: adrpo, francois.beaude@…

Description

When a function name is accidentally used twice in a module one might get cryptic messages: Either a not really helping
... Failed to elaborate expression ...
or a in some cases a more helpful C compiling error:
redefinition of ... with a different type ...
Which one depends one usage of the duplicate function.

Change History (20)

comment:1 Changed 8 years ago by sjoelund.se

  • Component changed from MetaModelica to Frontend
  • Owner changed from sjoelund.se to somebody

We used to have detection of duplicate classes, but it seems we no longer do. Try:

model M
end M;

model M
end M;

comment:2 Changed 8 years ago by wbraun

  • Cc adrpo added

That issue might lead to really annoying behaviour and
not really helping error messages.
Case1:

loadString("
model M
  Real y = sin(time);
  Real x = cos(time);
end M;
 // some pages down [...]
model M
  Real x = sin(time)*y;
  Real z=2;
end M;
");

One might wonder why model M does not have variable z.

Case2:

loadString("
package p
 model xy
   Real x = sin(time)*der(x);
   Real y = sin(time);
 end xy;

 // some pages down [...]

 model xy
   Real x = cos(time)*der(x);
   Real y = cos(time);
   //Real z = x*y;
 end xy;
end p;
");

In this second case we throw a quite cryptic error message

Error: Internal error Env.avlTreeReplace failed
Error: Error occurred while flattening model p.A

which occurs only if you activate the line with
variable z and it's quite hard to find out that
the mistake was done before and not by this line.

Last edited 8 years ago by wbraun (previous) (diff)

comment:3 Changed 8 years ago by adrpo

  • Owner changed from somebody to perost
  • Status changed from new to assigned

I remember we did this check right after parsing via SCodeUtil. I'll ask Per to have a look.

comment:4 Changed 8 years ago by perost

  • Status changed from assigned to accepted

The check should only be done for classes that are used, i.e. when building a class scope. There's already code for that in FNode.avlTreeAdd2, but it's commented out. But enabling the code causes a lot of false positives, it seems the same elements are added multiple times in some cases. We probably just need to use e.g. avlTreeReplace instead in some places. I will investigate further next week.

comment:5 Changed 7 years ago by casella

  • Cc francois.beaude@… added
  • Milestone changed from Future to 2.0.0
  • Priority changed from normal to blocker

Another test case reported in #4211, which is closed as duplicate.

I think this should really be fixed for 2.0.0. We cannot afford to have unpredictable or cryptic beaviour of the compiler when there are duplicate class definitions in the code. This case should be explicitly reported as an error.

comment:6 Changed 7 years ago by perost

Fixed in r860bb91 and 669113d. The check for duplicate elements is disabled for MetaModelica though, because otherwise the bootstrapping fails. This is caused by actual duplicate elements, see Hudson log. Maybe Martin wants to take a look at that.

comment:7 Changed 7 years ago by perost

  • Priority changed from blocker to high

comment:8 Changed 7 years ago by sjoelund.se

For bootstrapping, it seems to be the interface files when dumped since they use SCode. Any type variables are duplicated since Absyn to SCode fixes some classes...

encapsulated uniontype DoubleEndedList<T>  

to:

encapsulated uniontype DoubleEndedList<T>  
  final type T = polymorphic<Any>;

comment:9 Changed 5 years ago by casella

  • Component changed from Frontend to New Instantiation

comment:10 Changed 5 years ago by casella

  • Milestone changed from 2.0.0 to 1.14.0
  • Priority changed from high to blocker

I tried loading this package:

package p
 model xy
   Real x = sin(time)*der(x);
   Real y = sin(time);
 end xy;

 // some pages down [...]

 model xy
   Real x = cos(time)*der(x);
   Real y = cos(time);
   //Real z = x*y;
 end xy;
end p;

in OMEdit. The package is loaded without complaining, which is probably not good. When I expand the package, two classes with the same name are shown, which is odd. However, the following error message is displayed:

[1] 10:24:44 Translation Notification
[p: 2:2-5:8]: From here:

[2] 10:24:44 Translation Error
[p: 9:2-13:8]: An element with name xy is already declared in this scope.

However, it's really not clear which of the two xy classes will be used.

If I load package p with loadString in OMShell, I get no error messages whatsoever.

I think this should be improved, let's wait to see what happens with Adrian's new API. It would be best if we could fix this for 1.14.0

comment:11 follow-up: Changed 5 years ago by casella

@perost what is the status with this issue?

comment:12 in reply to: ↑ 11 Changed 5 years ago by perost

  • Component changed from New Instantiation to OMEdit
  • Owner changed from perost to adeas31
  • Status changed from accepted to assigned

Replying to casella:

@perost what is the status with this issue?

The status is that I don't know why this is still assigned to me, I fixed the check in the OF earlier while the NF didn't need any fixing. So neither frontends will instantiate p.xy due to the duplicated elements. The remaining issues seems to be OMEdit and/or API related.

comment:13 Changed 5 years ago by casella

OK, thanks!

comment:14 Changed 5 years ago by adeas31

  • Component changed from OMEdit to Interactive Environment
  • Owner changed from adeas31 to adrpo

I think its more of an API issue then OMEdit.

comment:15 Changed 4 years ago by casella

  • Milestone changed from 1.14.0 to 1.15.0

Releasing 1.14.0 which is stable and has many improvements w.r.t. 1.13.2.

This issue, previously marked as blocker for 1.14.0, is rescheduled to 1.15.0

comment:16 Changed 4 years ago by casella

  • Milestone changed from 1.15.0 to 1.16.0

Release 1.15.0 was scrapped, because replaceable support eventually turned out to be more easily implemented in 1.16.0. Hence, all 1.15.0 tickets are rescheduled to 1.16.0

comment:17 Changed 3 years ago by casella

  • Milestone changed from 1.16.0 to 1.17.0

Retargeted to 1.17.0 after 1.16.0 release

comment:18 Changed 3 years ago by casella

  • Milestone changed from 1.17.0 to 1.18.0

Rescheduled to 1.18.0

comment:19 Changed 3 years ago by casella

  • Milestone 1.18.0 deleted

Ticket retargeted after milestone closed

comment:20 Changed 3 years ago by casella

  • Milestone set to 1.19.0

1.18.0 blocker tickets moved to 1.19.0

Note: See TracTickets for help on using tickets.