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

Symbol should expose the actual symbol name #28372

Closed
Hixie opened this issue Jan 13, 2017 · 14 comments
Closed

Symbol should expose the actual symbol name #28372

Hixie opened this issue Jan 13, 2017 · 14 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue core-2 library-core library-mirrors type-enhancement A request for a change that isn't a bug

Comments

@Hixie
Copy link
Contributor

Hixie commented Jan 13, 2017

It knows it, since it's in the toString, but it's not easily extracted.

@matanlurey
Copy link
Contributor

This is with the realization that the actual name might be 'v' with minification applied, right?

(There is a way to do this with dart:mirrors - but that is for getting the "real name")

@Hixie
Copy link
Contributor Author

Hixie commented Jan 13, 2017

In my case I only care about the VM case, in particular the VM case in checked mode, where there's never minification as far as I know. I agree that this would be less useful in other modes.

In the context where I'm using this (Flutter's flutter_test test harness), we don't have dart:mirrors unfortunately.

Having said all that, if dart:mirrors can do it, why couldn't Symbol just do it itself? If dart:mirrors can do it then the data is there, right? Minification or no?

@rmacnak-google rmacnak-google added library-core library-mirrors closed-as-intended Closed as the reported issue is expected behavior labels Jan 13, 2017
@rmacnak-google
Copy link
Contributor

This is by design. Mirrors traffick in Symbols instead of regular Strings to make minification easier for dart2js, by requiring one to both import 'dart:mirrors' and use a static method perform conversions.

@eernstg
Copy link
Member

eernstg commented Jan 13, 2017

If dart:mirrors can do it then the data is there, right? Minification or no?

The typical situation (dart2js compilation is one example) is that import 'dart:mirrors'; causes compilation to preserve more information about the program than would otherwise have been necessary. So the data can be made available if you tell the compiler that it must be available. The downside is that this may turn a 14kB program into a 1.9MB program (comment out one or the other declaration of showName):

import 'dart:mirrors';

class HelloWorld {}

String showName(o) => "HelloWorld";

String showName(o) {
  TypeMirror oType = reflect(o).type;
  return MirrorSystem.getName(oType.simpleName);
}

main() {
  var x = new HelloWorld();
  print(showName(x));
}

Even though the vm may have the information in any case, it wouldn't be able to deliver it "for free" without destroying the portability of the code. So you have to use mirrors to get at this information.

@Hixie
Copy link
Contributor Author

Hixie commented Jan 13, 2017

I feel like this is a "least common denominator" decision, where the VM experience suffers because of a limitation of one of the other targets.

The reality of the situation is that I need to make the Flutter test harness error messages usable, which means showing the actual names of the symbols that I collect from a mock that is collecting Invocations via noSuchMethod. Today I do this by extracting the name from the Symbol.toString() using very brittle substring manipulation. I think it would be better if, when the information is available, it was exposed via a cleaner API.

Since the use case is for tests only, it doesn't matter if it only works in the checked mode, non-AOT, non dart2js target that flutter test uses. Yes the code isn't portable, but we've already crossed that bridge since the code depends on dart:ui which isn't available anywhere else either.

@lrhn
Copy link
Member

lrhn commented Jan 13, 2017

We do expose the name in the toString. We should either stop doing that, or admit that we expose it and make a better API for it (e.g., a name getter). Right now we effectively have a string based API that requires the user to parse the string to extract the available information.

Sadly, the same argument goes for the toString methods of Type and Object.

That doesn't meant that we have to use the un-minified name in a minified program. We can return the minified name, if that is what we show in the toString result. We might want to, but we don't have to. A minified program that doesn't create new symbols from strings at runtime, and which doesn't use MirrorSystem.getName(symbol), can just keep only the minified names. Not particularly readable, but you did ask to have your program minified!

@lrhn lrhn reopened this Jan 13, 2017
@lrhn lrhn added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. enhancement and removed closed-as-intended Closed as the reported issue is expected behavior labels Jan 13, 2017
@tvolkert
Copy link
Contributor

Sadly, the same argument goes for the toString methods of Type and Object.

And Function too

@matanlurey
Copy link
Contributor

+1 for web use cases too

I'm OK with mangled names with optimizations enabled, but dart2js with minifications disabled or DDC should show the full name/symbol (for debugging), and the name can be horribly mangled in dart2js with minification enabled.

I'd like to see:

  • Symbol.name
  • Function.name
  • Type.name*

*There is already a lot of code that uses the Type, especially for testing:

abstract class SuperService {}

// If someone refactors the name SuperService, it also applies here.
test('$SuperService should never throw an exception', () {
  ...
});

@rmacnak-google
Copy link
Contributor

I'm not sure it will help, but it is possible for Flutter to expose the functionality of MirrorSystem.getName through a function in another core library such as dart:ui. E.g.,

library dart.ui;
import 'dart:_internal' as internal;

String unwrapSymbol(Symbol symbol) => internal.Symbol.getUnmangledName(symbol);

which would work in all VM modes.

@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed enhancement labels Feb 9, 2017
@floitschG floitschG added core-2 and removed core-m labels Aug 21, 2017
@rrousselGit
Copy link

Any updates on this?

For code-generations purpose, I would like get the name of functions/classes/variables. I can parse the toString but that's not ideal

@lrhn
Copy link
Member

lrhn commented Aug 14, 2018

No changes here. As for updates, dart2js no longer supports dart:mirrors, so there is no guaranteed way to convert symbols to strings on the web. You can no longer implement Function, so now it's technically possible to add a name getter to Function and have all function types support it (even those with no name).

If anything, I'd put introspection/debugging functionality into dart:developer, rather than piggy-back it on the run-time values derived from the named declarations.

For code generation running on the VM, just use dart:mirrors.

@a-siva a-siva added closed-as-intended Closed as the reported issue is expected behavior closed-not-planned Closed as we don't intend to take action on the reported issue and removed closed-as-intended Closed as the reported issue is expected behavior labels Aug 5, 2022
@a-siva a-siva closed this as completed Aug 5, 2022
@Hixie
Copy link
Contributor Author

Hixie commented Aug 17, 2023

FWIW, Flutter continues to just depend on Symbol.toString:

// Workaround for https://github.com/dart-lang/sdk/issues/28372
String _symbolName(Symbol symbol) {
  // WARNING: Assumes a fixed format for Symbol.toString which is *not*
  // guaranteed anywhere.
  final String s = '$symbol';
  return s.substring(8, s.length - 2);
}

One change since this bug was filed is that this hack is now part of our public API (the mock canvas logic is now part of flutter_test instead of being just an internal thing we use ourselves).

@lrhn
Copy link
Member

lrhn commented Aug 23, 2023

Have you considerd just hard-coding the symbols that occur in the Canvas class declarations?

Something like: lrhn/flutter@b94305e

(Map auto-generated by a very simple Dart script, from the source code of the canvas.dart file. Got away with being naive because the Canvas API is remarkably simple.)

@Hixie
Copy link
Contributor Author

Hixie commented Aug 24, 2023

That seems vastly less ergonomic and harder to maintain :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue core-2 library-core library-mirrors type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests